Arguments #101
Arguments #101
Conversation
default argument / edit on interactive / edit on others
|
Hi Julian, I had problem with conflicts on the version on had on my account and the version I imported from the main repo. So I deleted my own fork and did a fresh fork. So my commits here are based on the latest package release. To decrease my mistakes with GIT, I will separate each of my tries in a new branch and will use a little more simple functions of the GIT. Please advise me over the updates and guide me through the next steps and the desired upgrades. I believe we can add a progress bar afterwards. |
|
I also did tests using virtual environment for the python, as your described for me in your other comments. It made the test and functionality really fast. Thanks! |
There was a problem hiding this comment.
That was fast! Well done on keeping these changes on a separate branch. The --visual argument is an interesting idea, though I think you may have bitten off more than you can chew at this time.
Hi Julian, I had problem with conflicts on the version on had on my account and the version I imported from the main repo. So I deleted my own fork and did a fresh fork.
Sorry to hear that. I guess it was not too much of a loss this time, but next time you run into such troubles, feel welcome to ask for help. Generally, such conflicts can be fixed without any loss of data.
So my commits here are based on the latest package release. To decrease my mistakes with GIT, I will separate each of my tries in a new branch and will use a little more simple functions of the GIT.
Good plan!
For now, there is just one thing you need to fix in your branches. It seems you accidentally advanced your develop with the first commit on your arguments branch. You can rewind your develop as follows, so it realigns with develop on my repo:
git checkout develop
git reset --hard upstream/develop
git push --force origin developPlease advise me over the updates and guide me through the next steps and the desired upgrades. I believe we can add a progress bar afterwards.
I will certainly guide you. Baby steps, though!
I also did tests using virtual environment for the python, as your described for me in your other comments. It made the test and functionality really fast. Thanks!
Please remind me, what did I write about that? Probably unrelated, but: when I try the tests on your code, they fail.
So, now for the actual review. I like some of the things you did, but I'm afraid you got ahead of yourself and made a bit of a mess in the process. Most importantly:
- You made unnecessary breaking changes.
pip-reviewwithout arguments no longer shows available updates, which is causing the tests to fail and would certainly frustrate lots of users. In addition, the precedence of the--rawand--autoarguments switched, which is likely to break some people's scripts. - You lost track of the possible interactions between the arguments. There are many ways to make
pip-reviewdo convoluted or even nonsensical things.
You need to exercise minimalism. What I would like you to do (please ask for help if you run into any difficulty at any step):
- Read my comments below, but don't follow up on them yet. Just make some mental notes.
- Make a copy of your current
argumentsbranch so you can look back at it if necessary.
git branch arguments-bak arguments- Rewind the
argumentsbranch all the way back toupstream/develop. In the next steps, you will start over, making small improvements on top of the work of @kunif.
git checkout arguments
git reset --hard upstream/develop- First problem to solve:
pip-review --preview --rawwill show both a table and the raw format, which is pointless. For a human reader, the raw format adds no information and is less readable than the table. For a machine, only the raw format is parsable. Try to make the smallest possible code change so thatpip-review --preview --rawbehaves exactly likepip-review --preview; in other words, so that--rawis ignored if--previewis also passed. Do not change any other behavior. Do not continue before you have solved this problem and committed your changes. - Second problem to solve:
pip-review --previewwill show both a table and the wordy format, which is also pointless. Solve this problem, again trying to find the smallest possible code change and without changing any other behavior. Succeed and commit before continuing. - With your previous change in place, the
--preview-onlyargument is redundant. Remove it. Make sure you update all parts of the code that have something to do with the removed argument. Commit again. - Force-push the new version of your
argumentsbranch. This will update the current pull request. - Let me know that you are ready for another review.
- Create a new issue ticket for your idea for the
--visualargument. I think we need to discuss it.
Please let me know if you have any questions or comments!
| @@ -57,7 +57,7 @@ Example, preview for update target list by ``pip list --outdated`` format, with | |||
| $ pip-review --auto --preview | |||
| ... <same above and pip install output> | |||
|
|
|||
| Example, only preview for update target list: | |||
| Example, preview a table of the update target list, which is a combination of the interactive and preview formats: | |||
There was a problem hiding this comment.
This is not a combination, is it? I think the next example is just the preview (table) format.
There was a problem hiding this comment.
Also, note that it is still saying --preview-only on line 64.
| @@ -69,30 +69,62 @@ def parse_args(): | |||
| description=description, | |||
| epilog=EPILOG+version_epilog(), | |||
| ) | |||
|
|
|||
| # ---------------------------- | |||
There was a problem hiding this comment.
I'm in favor of adding a blank line here. The ruler is maybe not so necessary. If you want to keep the ruler, please give it the same indentation as the surrounding code.
| ''' example: | ||
| ezdxf==1.0.2 is available (you have 0.8.0) | ||
| pip==23.0.1 is available (you have 22.0.4) | ||
| setuptools==67.3.2 is available (you have 58.1.0) | ||
| ''' |
There was a problem hiding this comment.
This is a nice touch, makes the meaning of the arguments more tangible for the programmer.
This comment is in the wrong place, though; --verbose does not mean "show me the wordy format" but it means "show all output with logger.debug in addition to all output with logger.info". It so happens that pip-review does not have such output anymore; all former calls to logger.debug were in Vincent Driessen's custom logic to compute the outdated packages, which fell out of the codebase when we transitioned to a thin wrapper around pip list --outdated in version 1.0.0. It is only now that I realize that the --verbose flag doesn't make any difference anymore because of this!
Anyway, the comment you wrote here would probably be more appropriate on line 73, where you put the ruler. This is because the wordy format is the default format, which appears if you don't pass --raw or --preview. You could further clarify this by changing the header from just example: to default format:.
Oh, and one more thing: it is conventional to use double triple quotes for comments, so """ instead of '''.
| ''' example: | ||
| ezdxf==1.0.2 | ||
| pip==23.0.1 | ||
| setuptools==67.3.2 | ||
| ''' |
| ''' exmple: | ||
| Package Version Latest Type | ||
| ------------------------------- | ||
| ezdxf 0.8.0 1.0.2 wheel | ||
| pip 22.0.4 23.0.1 wheel | ||
| setuptools 58.1.0 67.3.2 wheel | ||
| ------------------------------- | ||
| ''' |
There was a problem hiding this comment.
Again correct, and nice for consistency. Would be even better if you move the argument and this comment up, so they come directly after --raw and its comment. That would group all the formats together.
| if args.verbose: | ||
| for pkg in outdated: | ||
| logger.info('{0}=={1} is available (you have {2})'.format( | ||
| pkg['name'], pkg['latest_version'], pkg['version'] | ||
| )) |
There was a problem hiding this comment.
Here, you're changing the meaning of the --verbose flag. Please don't change the meaning of already released flags!
| for pkg in outdated: | ||
| logger.info('{0}=={1} is available (you have {2})'.format( | ||
| pkg['name'], pkg['latest_version'], pkg['version'] | ||
| )) |
There was a problem hiding this comment.
Also, this same snippet of code now appears in three places. Keep it DRY: Don't Repeat Yourself!
| # --raw | ||
| # --interactive |
There was a problem hiding this comment.
These comments don't really add information; it already says if args.raw and args.interactive on the next line.
| else: | ||
| logger.info('You chose to quit the update process') | ||
|
|
||
| return |
There was a problem hiding this comment.
There are way too many changes on the past 80 lines. Firstly, because you did not need to make so many changes, secondly, because some of the changes are disruptive for users, and thirdly, because you lost track of all the possible interactions between the options. Let us compare pip-review's behavior in the past to what these changes made of it.
Behavior in all published releases so far (up to 1.3.0), by order of precedence of options:
pip-review- report: wordy
- updates: none
pip-review --auto- report: no output
- updates: all
pip-review --raw- report: raw
- updates: none
pip-review --interactive- report: wordy (combined with selection)
- updates: interactive selection
--verbosecombined with any of the above: would give diagnostic output, if our code would actually use that
Behavior on develop (after changes by @kunif):
pip-review- report: wordy
- updates: none
pip-review --preview-only- report: table
- updates: none
pip-review --preview- report: table
- updates: none, but repeats all available updates in wordy format
pip-review --preview --auto- report: table
- updates: all
pip-review --preview --raw- report: table
- updates: none, but also raw report shown
pip-review --preview --interactive- report: table
- updates: interactive selection (in wordy format)
pip-review --auto- report: no output
- updates: all
pip-review --raw- report: raw
- updates: none
pip-review --interactive- report: wordy (combined with selection)
- updates: interactive selection
--verbosecombined with any of the above: would give diagnostic output, if our code would actually use that
Behavior that I think would make most sense (nearly the same but no --preview-only and no --preview --raw):
pip-review- report: wordy
- updates: none
pip-review --preview- report: table
- updates: none (as
--preview-onlyon currentdevelop)
pip-review --preview --auto- report: table
- updates: all
pip-review --preview --interactive- report: table
- updates: interactive selection (in wordy format)
pip-review --auto- report: no output
- updates: all
pip-review --raw- report: raw
- updates: none
pip-review --interactive- report: wordy (combined with selection)
- updates: interactive selection
--verbosecombined with any of the above: would give diagnostic output, if our code would actually use that
Behavior after your changes in d05f518..d0c7792:
pip-review- nothing happens, this is a breaking change!
pip-review --verbose- report: wordy
- updates: none
- also enables diagnostic input as before
pip-review --verbose --raw- report: wordy
- updates: none, but also raw report shown
pip-review --verbose --interactive- report: wordy
- updates: interactive selection (each available update mentioned twice)
pip-review --verbose --interactive --auto- report: wordy
- first round of updates: interactive selection (each available update mentioned twice)
- second round of updates: all (based on situation before previous round)
pip-review --verbose --interactive --preview- first report: wordy
- updates: interactive selection (each available update mentioned twice)
- second report: table (based on data before interactive update)
pip-review --verbose --interactive --preview --visual- first report: wordy
- first round of updates: interactive selection (each available update mentioned twice)
- second report: table (based on data before interactive update)
- second round of updates:
- interactive selection (still based on data before previous interactive update)
- selection displayed in a table
- confirmation
pip-review --verbose --interactive --visual- report: wordy
- first round of updates: interactive selection (each available update mentioned twice)
- second round of updates:
- interactive selection (based on data before previous interactive update)
- selection displayed in a table
- confirmation
pip-review --verbose --auto- report: wordy
- updates: all
pip-review --verbose --preview- first report: wordy
- second report: table
pip-review --verbose --preview --visual- first report: wordy
- second report: table
- updates:
- interactive selection (each available update mentioned three times)
- selection displayed in a table
- confirmation
pip-review --verbose --visual- report: wordy
- updates:
- interactive selection (each available update mentioned twice)
- selection displayed in a table
- confirmation
pip-review --raw- report: raw
- updates: none
pip-review --interactive- report: wordy (combined with selection)
- updates: interactive selection
pip-review --interactive --auto- report: wordy (combined with selection)
- first round of updates: interactive selection
- second round of updates: all (based on situation before previous round)
pip-review --interactive --preview- first report: wordy (combined with selection)
- updates: interactive selection
- second report: table (based on data before interactive update)
pip-review --interactive --preview --visual- first report: wordy (combined with selection)
- first round of updates: interactive selection
- second report: table (based on data before interactive update)
- second round of updates:
- interactive selection (still based on data before previous interactive update)
- selection displayed in a table
- confirmation
pip-review --auto- report: no output
- updates: all
pip-review --preview- report: table
- updates: none
pip-review --preview --visual- report: table
- updates:
- interactive selection
- selection displayed in a table
- confirmation
pip-review --visual- updates:
- interactive selection
- selection displayed in a table
- confirmation
- updates:
|
|
||
| if __name__ == '__main__': | ||
| try: | ||
| main() | ||
| except KeyboardInterrupt: | ||
| sys.stdout.write('\nAborted\n') | ||
| sys.exit(0) | ||
| sys.exit(0) |
There was a problem hiding this comment.
Don't strip newlines from the ends of files! Some tools rely on those newlines being there.
Thank you, yes I got lost between the upstream, master, branches and all. The easiest way to get rid of the hell was to do a clear fork. I remember the first days I was using the VIM and NVIM. The cross icon on top of the terminal window was my scape way! :)) Sure I will ask you any questions from now on. It is not wise to delete my whole fork as I will miss my content and progress.
I think I again messed my fork!
I read the 'virtual environment' here (link), so I made 'venv's to do my checks in virtual environments.
I think I have solved this issue. This is the output of the
Sure will do it. I am replacing it with the visual arguement.
Sure will do it after the developments.
Sure will do it. Many thanks for all the comments. I am afraid I lose track of your points, but please be patient, I am not a professional software engineer. I am a civil engineer. So my speed may be not as much as expected. But I am working on this, in my free time. |
Ah, I see what's missing. You did not add my public repository as a remote yet. Do this first: git remote add -f upstream https://github.com/jgonggrijp/pip-review.gitThen you can
Ah, the performance tests. Now I remember.
Agreed. After adding the
What do you mean by "polishing the arguments"? As far as I'm concerned, fixing the interactions between arguments (as well as their semantics alone) is polishing the arguments.
You are right, you fixed this. However, I want you to start over and fix it with the smallest possible code change. Your current set of changes causes a lot of new trouble as a side effect!
For now, please just remove it. Save the
Sure, no worries and no hurry. Everyone needs to start from zero and I am doing this in my spare time, too. Let's just focus on getting the branches right, first. |
I have this in mind: I assume their semantics alone is the first priority and then we solve the interactions between arguments. I have a sense maybe if we are not sure about the arguments themselves, we may not successfully approach the interactions. Maybe we approach the interactions and later we need to fix something in semantics themselves that may affect the interactions. That will be a duplicate work. May be I am wrong, not sure.
Sure will open a new ticket for this and will archive it.
Good, Sure I will work on the branches for now. |
|
If you first want to fix |
|
Got it, sounds good. This way we have a list of actions and steps. I will update you on my progress.
|
Focus on these updates are on the arguments.