Conversation
|
@scorphus Is everything good with my pr, would you like me to enhance anything? 😄 |
scorphus
left a comment
scorphus
left a comment
There was a problem hiding this comment.
Thanks for contributing, @gkodosis!
At a first look:
- The changes cause tests to fail
- There're no changes to the README.md file
- There are no tests for the changes introduced
- The special cases in the original rule are lost
I think a general rule is less useful than a specific one. Considering a specific rule, it is already hard for TheFuck to get a valid, useful recommendation. Let alone a general one.
So, IMO, the original rule should be kept as is and additional rules should be created for each of those other commands, each of them with their particularities (if any). Also, each of them with enabled_by_default properly set, including the original—that being an improvement.
What do you think?
|
Yes I totally agree with all your points. My only thoughts lie on the matter of the global rule. Due to the almost identical commands of the env packages, my solution seemed really convenient. Though, if you think is better to make separate files for each command, I will work on it for sure. Probably the testing process will be more efficient in this way as well. |
|
The identical parts of the rules could be kept in a single, separate “ |
|
Yes, that' s a great idea! I will work on it, as well as the requested changes and come up with a new pr. |
|
Need to check why the tests fail, otherwise I think I managed to fullfill the requested changes and they seem really great! |
|
@scorphus Hello again, everything seems to work pretty great! Unfortunately, your idea of integrating some of the common code of the rules to specific/devenv.py was making the tests to fail, so I decided to simplify it. Hope I find some extra time and manage to integrade more of the common code of the rules in a couple of weeks. Thank you for your help! |
|
You're on fire! 🔥 I hope I soon have time to review it more thoroughly. Thanks! |
|
Hello @scorphus I didn't have any time to enhance this commit, as I can understand you haven't found any time to review it in order to merge it as well. Let me know about your status 😎 |
scorphus
left a comment
scorphus
left a comment
There was a problem hiding this comment.
Hey, @gkodosis, sorry for taking long to review.
Please, accept my apologies. I completely overlooked the fact that the commands of all virtual env managers are practically identical.
I guess I'll reset to one of your previous commits and make a couple small adjustements.
scorphus
force-pushed
the
master
branch
2 times, most recently
from
f24735b to
57e7e37
Compare
|
Hey @scorphus, It's okay don't worry! Let me know if you need any help, I will be glad to help 😃 |
|
I'd love if you could spare a review 🙌 |
Co-authored-by: Divy Jain <[email protected]>
Co-authored-by: Divy Jain <[email protected]>
|
Sorry guys I was pretty busy and saw the updates just now, cheers @divykj for the review! Glad to help @scorphus 🙌 |
3 participants
Getting pyenv_no_such_command.py rule as base, I created an env_no_such_command.py script able to hanlde all env related commands. Transforming, in this way, the pyenv rule in a more global one.
This commit was originated and fixes #1074