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

Python Support #18

Merged
merged 20 commits into from
Mar 31, 2020
Merged

Python Support #18

merged 20 commits into from
Mar 31, 2020

Conversation

mr2rm
Copy link
Contributor

@mr2rm mr2rm commented Mar 27, 2020

Hi,
Recently after a long time away from competitive programming, I'v decided solving some programming challenges everyday. So I searched for the available tools which I can use to do it better and have more fun. Finally I've found the competitive-programming-helper useful to test my solutions.

Yesterday, I've tried competitive-programming-helper in CodeForces contest and I had to solve problems only with C and C++. Since I'm a Python lover (of course beside C++ and JavaScript) and I use it a lot to solve my daily challenges, I decided to equip it to Python.

So because it's the starting point of support multi languages, I made some fundamental changes to take effect. Here are my changelogs:

  • Add a QuickPick to get the user-selected language (C, C++ and Python)
  • Add language configuration to handle multi languages
  • Some changes to introduce .py as a valid extension (e.g. keybindings, source creator, test runners and etc.)
  • Implement Python compilation process
  • Update test case runners to support Python
  • Some other changes and refactors to handle multilanguage support

Any feedback and response would be appreciated. Also I hope these changes will be useful and could help everybody who wants to improve his problem solving paradigm.

@agrawal-d agrawal-d self-requested a review March 27, 2020 14:09
@agrawal-d agrawal-d added the enhancement New feature or request label Mar 27, 2020
@agrawal-d
Copy link
Owner

@mr2rm Thanks a lot for the PR! I'm very happy to see contributions from others 😄
Before I review this, let me try to fix these rebase issues, it appears some older commits are becoming a part of this PR.
Once again, thanks for working on this!

@agrawal-d agrawal-d self-assigned this Mar 27, 2020
@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 27, 2020

@mr2rm Thanks a lot for the PR! I'm very happy to see contributions from others
Before I review this, let me try to fix these rebase issues, it appears some older commits are becoming a part of this PR.
Once again, thanks for working on this!

Don't mention it :))
I had some other ideas to improve it. I'll work on them in near future.
Oops! What's wrong with it?

@agrawal-d
Copy link
Owner

agrawal-d commented Mar 27, 2020

Okay, it appears you did not work on the latest master. Can you please rebase onto the latest master and fix any merge conflicts?
You would do something like:

$ git fetch origin

$ git rebase origin/master

Now you will get come merge conflicts. Fix each of the conflicted file, after fixing the conflict, use

$ git add filename

After doing this for all the files, do

$ git rebase --continue

Then, to push them to the PR, do

git push mr2rm python-support --force

This will remove the unnecessary extra commits.
If you are uncomfortable with git, I'll be happy to do it for you.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 27, 2020

Okay, it appears you did not work on the latest master. Can you please rebase onto the latest master and fix any merge conflicts?
You would do something like:

$ git fetch origin

$ git rebase origin/master

Now you will get come merge conflicts. Fix each of the conflicted file, after fixing the conflict, use

$ git add filename

After doing this for all the files, do

$ git rebase --continue

Then, to push them to the PR, do

git push mr2rm python-support --force

This will remove the unnecessary extra commits.
If you are uncomfortable with git, I'll be happy to do it for you.

I've already merged upstream into master and python-support in 2bbdbb0 and b94742e commits. I'm ok with it. I don't know how this thing happened.

@agrawal-d
Copy link
Owner

I've already merged

That's exactly the problem - in this codebase, we use rebase, so if you merge, it will create extra commits. Don't worry, it's not a big deal. Let me know if you face any problems fixing this.

@agrawal-d
Copy link
Owner

agrawal-d commented Mar 27, 2020

I think you should also delete those two merge commits - b94742ed9a331c6f9ac0b62fac62ab5b92ca5ac3 and 2bbdbb0efc65bde352794561d895c56ce8f3c20b.

(using interactive rebase).

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 27, 2020

I think you should also delete those two merge commits - b94742e and 2bbdbb0.

(using interactive rebase).

I've deleted those two commits both from master and python-support.

$ git fetch origin

Do you mean upstream?

@agrawal-d
Copy link
Owner

Do you mean upstream?

Whatever's the name of the branch that points to this repo. Yeah, most likely upstream.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 27, 2020

@agrawal-d Rebase issues has just resolved and it's ready to review. ‌I think everything is OK. Tell me if there was any other problem.

@agrawal-d agrawal-d linked an issue Mar 28, 2020 that may be closed by this pull request
Copy link
Owner

@agrawal-d agrawal-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A few comments below.

compileFile.js Show resolved Hide resolved
compileFile.js Outdated Show resolved Hide resolved
compileFile.js Show resolved Hide resolved
compileFile.js Show resolved Hide resolved
config.js Outdated Show resolved Hide resolved
extension.js Show resolved Hide resolved
extension.js Outdated Show resolved Hide resolved
extension.js Outdated Show resolved Hide resolved
locationHelper.js Outdated Show resolved Hide resolved
extension.js Outdated Show resolved Hide resolved
@agrawal-d
Copy link
Owner

@mr2rm I replied to your comments.

@agrawal-d
Copy link
Owner

Let me know when you're done with making the changes so that I can merge this PR.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 28, 2020

@agrawal-d At first impression I wanted to separate python2 and python3 but when I came to it later which It's the better idea to use default version of the host. So it needs to manage the default version if needed.
But now with these requested changes we have below options to decide about it:

  1. Only python3 will be supported
  2. Use the default version of host but check it at runtime and set flags based on the version
  3. Separated versions and user can choose any of them (If selected option is not available, a message will be showed and let him to select another one)
  4. Separated versions but only available versions will be listed

Any other options? Which one do you prefer?

@agrawal-d
Copy link
Owner

I prefer 1, i.e. Only python3 will be supported. I don't want us to maintain lots of checking code etc.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 28, 2020

I prefer 1, i.e. Only python3 will be supported. I don't want us to maintain lots of checking code etc.

@agrawal-d I agree with you and Python 2 will be deprecated soon 😄
What should we do if user selected Python but python3 is not available?

@agrawal-d
Copy link
Owner

Right now we don't do anything if gcc or g++ is not installed. I guess, we can show an info box in the corner if any of the compilation fails due to compiler/interpreter not being installed.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 28, 2020

@agrawal-d What about to store .pyc files in binLocation?
If we gonna do it, we have to move it manually after the creation. I couldn't find any other solution to do that. There were any command line flag related to it.
But maybe we can do something for Python 3.2 or above. Let's check here for more details.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 28, 2020

I guess, we can show an info box in the corner if any of the compilation fails due to compiler/interpreter not being installed.

Good idea. I agree with you again 😄
Why don't you create a new issue for it?

@agrawal-d
Copy link
Owner

Why are we even generating the bytecode for Python? Let's just skip compilation for python and only run the interpreter to evaluate.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 28, 2020

Why are we even generating the bytecode for Python? Let's just skip compilation for python and only run the interpreter to evaluate.

We can do it but I prefer to generate bytecode for better performance. What's wrong with move bytecode to binLocation manually?
Also skip compilation don't change anything; because we have to check the language again in test runner to do something else for Python.

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 29, 2020

@agrawal-d Did you notice my comment?
#18 (comment)

@agrawal-d
Copy link
Owner

We can do it but I prefer to generate bytecode for better performance. What's wrong with move bytecode to binLocation manually?

There is no performance benefit for executing a bytecode, it only helps in reducing the compilation time when the same file is executed again.[1]

I suggest that you skip the bytecode generation for Python and directly execute it.

[1] See http://effbot.org/pyfaq/can-python-be-compiled-to-machine-code-c-or-some-other-language.htm & https://docs.python.org/3/glossary.html#term-bytecode

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 29, 2020

it only helps in reducing the compilation time when the same file is executed again.

I call it performance, not execution performance; I mean compilation performance.

Note that the main script executed by Python, even if its filename ends in .py, is not compiled to a .pyc file. It is compiled to bytecode, but the bytecode is not saved to a file. Usually main scripts are quite short, so this doesn’t cost much speed.

I agree with above part of the first article which you mentioned. First of all implemented solutions are usually short and also each problem has a few testcases. So bytecode generation cost is not expensive and we can ignore it.

@agrawal-d
Copy link
Owner

@mr2rm are you done with the changes?

@mr2rm mr2rm requested a review from agrawal-d March 30, 2020 22:19
@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 30, 2020

are you done with the changes?

Yep, It's ready to review. Let's check it!

This commit marks the final one to bring in support for Python3.
Copy link
Owner

@agrawal-d agrawal-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the revisions. I've pushed a small commit on top with some revisions, including those mentioned in the comments below.

prefrencesHelper.js Show resolved Hide resolved
processManager.js Show resolved Hide resolved
@agrawal-d agrawal-d merged commit b5cd5fb into agrawal-d:master Mar 31, 2020
@agrawal-d
Copy link
Owner

Merged! Thanks for working on this. The refactors you made really improve the quality of the code!

I'll add your name to the README to show your contribution!

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 31, 2020

Merged! Thanks for working on this. The refactors you made really improve the quality of the code!

Thanks, I’m glad I could help. 😄

I'll add your name to the README to show your contribution!

It's so kind of you. 🙏
When will you release the new version in VSCode Extensions?

@agrawal-d
Copy link
Owner

When will you release the new version in VSCode Extensions?

Today!

@mr2rm
Copy link
Contributor Author

mr2rm commented Mar 31, 2020

I'll add your name to the README to show your contribution!

Also don't forget to mention Python in the Dependencies.

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

Successfully merging this pull request may close these issues.

[Feature request] Python Support
2 participants