-
Notifications
You must be signed in to change notification settings - Fork 449
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
fix: Fixed dependency issue that causes pip command to raise error #877
Conversation
@PrashanthPuneriya I removed some dependencies from requirements. Also, after researching I found that "ensure_str" was included in six==1.12.0. This issue may be due to old virtualenv. This issue was introduced in I tested it with the same configuration provided by you. |
Codecov Report
@@ Coverage Diff @@
## develop #877 +/- ##
========================================
Coverage 95.99% 95.99%
========================================
Files 96 96
Lines 5287 5287
========================================
Hits 5075 5075
Misses 212 212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I am pretty sure we can do a more extensive audit and remove many more dependencies.
Yes, @SanketDG in my opinion it's never a good choice to copy whole pip freeze to requirements file as some versions can cause issues later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the requirements.txt gives following compatabiltiy issues:-
Though I get the compatability issues. It works fine :)
I found that "ensure_str" was included in six==1.12.0.
But, we still do I have six==1.11.0 in the requirements.txt file right? Though this fixed the issue I still don't have any idea why it did. Can you please explain?
While installing we have to use url-resolver, that will be taken into effect from October, 2020. Also, there were some dependencies and we don't need to manage their versions. I removed those dependencies so that pip can install the most updated version automatically. |
@PrashanthPuneriya I will remove this conflicts soon. |
Okay! But, what change has exactly solved this issue? |
This issue arises due to dependencies of awsebcli that were included in requirements. I removed those dependencies only. |
@PrashanthPuneriya compatibility issues are solved now. |
@SanketDG @PrashanthPuneriya can you please review it once? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I guess you could remove many more packages
Pinging @PrashanthPuneriya to see if this is okay to go ahead with
@PrashanthPuneriya already open a new issue for that. If someone don't solve it then I will remove and update the dependencies. Here, I focus only on those that might create a problem for six |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have tested this locally and it works as expected. Haven't encountered any issues. We can go ahead with this.
With this PR we have fixed all the errors we used to encounter while setting up the project @SanketDG @isabelcosta :)
Thanks a lot for your contribution @KapilBansal320 :)
5ceb31f
Feel free to change the commit message and PR title to something more explanatory We have a Commit Message Style Guide |
@PrashanthPuneriya @SanketDG I made the necessary changes as asked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@isabelcosta Please review it |
I honestly don't think so, given just suggesting |
f0e8acf
@SanketDG the problem occurs in old versions of virtualenv. If we are proceeding by making virtualenv first (as explained in docs), then why
|
That pip is installed generally as root, so if you are not the root user, you have to use |
|
No, because then you'd be already inside the virtualenv, right? |
Okay @SanketDG should I remove changes in README.md?? Although this is not only for Linux and they may use sudo if needed while updating virtualenv |
I think the code block changes are good, just remove the installation step |
Fixed issue when pip package manager related commands raises error Updated the documentation Fixes #664
@SanketDG changes done now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@PrashanthPuneriya can you please test and verify that the pip dependency issues are solved now. Once it is done we can move it to ready to merge |
Tested locally and haven't encountered any dependency issues or any other issues. It is safe to merge this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good work @devkapilbansal !
I deployed this to a Heroku link and this worked fine 🎉
Description
Fixed issue when pip package manager related commands raise errors.
Fixes #664
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
python run.py
.python -m unittest discover tests
Checklist:
Code/Quality Assurance Only