Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ar786 patch 1 #47

Closed
wants to merge 26 commits into from
Closed

Ar786 patch 1 #47

wants to merge 26 commits into from

Conversation

AR786
Copy link
Contributor

@AR786 AR786 commented Dec 11, 2018

This request is for the issue #19

@rahulkumaran
Copy link
Owner

So one question. Why have you done more than what the issue asked you to do? Plus, it looks like you've removed the C testfiles.
Clearly mentioned in the issue that only the initialise statement logic needs to be there in the sudocode_pi.py! The reason why I wanted it that way was because I had some other plans to take this forward for python and give more people an opportunity to contribute. Plus, I want to provide multiple language support for this but you've removed all lines in main pertaining to the C code and modified the readme in such a way that it's more python oriented I believe. You've done some great work @AR786, but I'll have to close this because it contains a lot of changes that were not asked for. I'll give you some more time. Please read the issue carefully before doing the work from next time. All your changes are perfect, just that sudocode_pi.py contains the entire pseudocode conversion process. It becomes tough for me to review it entirely, which is another reason why I wanted to give this in parts. Please undo all your changes to README for now, also, I'd like to see the *.c files in the testfiles folder. Your new test cases for python can be named with a sudocode_py4.txt or something like that.

The execute_py.pi need not be a separate file. You could possibly add that logic to the code execution function in cleaner.py

@AR786
Copy link
Contributor Author

AR786 commented Dec 11, 2018

okay

@AR786 AR786 closed this Dec 11, 2018
@AR786 AR786 reopened this Dec 11, 2018
@AR786
Copy link
Contributor Author

AR786 commented Dec 11, 2018

are these fine ?

@rahulkumaran
Copy link
Owner

Cannot accept this PR. Everyone else's commit history will get erased. Plus 26 commits are too much.
Please refork this repo, and start using branches in a better manner. Don't make any changes to your master directly. The only changes to your master should be that changes on my master being pulled by you and pushed into your master. Otherwise, everytime you work on a PR, create a new branch. That would be better. This is something I can definitely not merge. The entire commit history will go. Sorry!

Also, I told you to work on just the initialise logic right? Please do send a new PR that contains only that logic in sudocode_pi.py and any other changes required in other files.
Please do send a new PR.

Sorry!

@AR786
Copy link
Contributor Author

AR786 commented Dec 13, 2018

@rahulkumaran bhaiya but last time you told just to bring back changes for readme and testfiles.
I will do all the things you told but can you please accept the sudocode_pi.py as a whole.
It took a lot of time. Please.
I will keep track from next time

@rahulkumaran
Copy link
Owner

I know you've worked hard but this is not about me or you. It's about the project. I don't really want to add such a huge change at once so I'm definitely not adding the whole thing @AR786. That's why I've constantly been telling everyone to read the issues carefully. Anyway, all these features will have to be implemented slowly, one after the other. So I could probably assign you to a couple directly. That's the maximum I can do.

Also, if you read my previous comment it's evident that I want you to get the logic only initialise.

Really sorry, but I can't really do much about this. The max I'll do is assign you directly to half of the python related issues. Does that sound okay with you?

@AR786
Copy link
Contributor Author

AR786 commented Dec 13, 2018

Okay bhaiya.. i will do just what you say.
By the way can you please once tell me exactly what should i have in sudocode_pi.py .
I will PR exactly what you want then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants