Improve argparser #87

cdce8p · 2021-01-18T23:48:27Z

Some improvements to the argument parser. This PR greatly improves the readability of the help command and introduces Enums for arguments.

Current help text

Changes

\n in help strings are now rendered correctly
Added argument groups and rearranged argument positions
Updated readme to match changes
Updated max_help_position to avoid unnecessary line breaks
Added choices as additional help for from, order and format
Added enums for from, order and format

--

I've split these changes into multiple commits to make reviewing them a bit easier.

On a related note, what is your opinion to squash merge for PRs? Although it might seem counterintuitive at first, I think it makes the git log much easier to read and the Github integration is better. The squash commit message includes the PR number which will be automatically be linked (for git blame).

* No code changes!

codecov · 2021-01-18T23:49:15Z

Codecov Report

Merging #87 (b7949b5) into master (606ff8b) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   98.87%   98.94%   +0.06%     
==========================================
  Files           1        1              
  Lines         357      380      +23     
==========================================
+ Hits          353      376      +23     
  Misses          4        4

Impacted Files	Coverage Δ
piplicenses.py	`98.94% <100.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 606ff8b...b7949b5. Read the comment docs.

raimon49 · 2021-01-19T11:29:00Z

@cdce8p Wow! Thank you. It's so easy to read!

I didn't know that you can do this hack with add_argument_group and self defined HelpFormatter. It's so cool.

what is your opinion to squash merge for PRs?

I like to commit to smaller units. That helps me in my review as well. Thanks.

I don't always squash merge approved PRs, but there is nothing wrong with squash merging this PR.

raimon49 · 2021-01-19T11:41:32Z

piplicenses.py

+        formatter_class=CustomHelpFormatter)
+
+    common_options = parser.add_argument_group('Common options')
+    format_options = parser.add_argument_group('Format options')


The argument_group title "Format options" looks a bit strange. Because option --format is in "Common options".

But I don't have a good alternative either 😅

By the way, "Verify options" is a great naming 👍

I agree, it's a bit strange. I can change the group names if you like, although I think it's good enough. The goal with Format options was to group all additional format arguments that one might not need that often.

cdce8p · 2021-01-19T12:01:50Z

I just remembered that I missed implementing a few small checks. I'll add them later today.
Those checks should cover specifying --no-license-path and --with-notice-file without also adding --with-license-file and --filter-code-page without --filter-strings.

cdce8p · 2021-01-19T19:14:37Z

Just pushed all remaining changes. In addition to extending args verification, I've also added some type hints for args. Those allow for much better autocompletions in IDEs. Lastly, I added a basic config for isort in setup.cfg. With that it's pretty easy to sort all imports and make them look nice.

After this PR is finished, I would like to propose moving from a single source file to a package structure. piplicenses.py is already quite large with almost 900 lines and it would only continue to grow in the future. The move could improve the readability. What do you think?

raimon49 · 2021-01-20T12:09:57Z

setup.cfg

+[tool:isort]
+# https://github.com/timothycrosley/isort
+multi_line_output = 4
+line_length = 72
+known_first_party =
+    piplicenses


@cdce8p Thank you for your great contribution. This is a very useful tool.

I have something to ask for future reference. Why do you want to overwrite the line_length setting from 79 (as default) to 72? I'd like to hear if there is any reason to recommend it.

To be honest, I don't really know. It's something I have picked up somewhere over time and it seems to work great. I guess an advantage would be that with only 72 characters not to many from imports are on one line, see test_piplicenses.py for example.

Regarding the default 79: Personally I would recommend to change it. I tend to use 119 for my projects. That doesn't mean that every line should now be longer, but that in some limited cases it improves readability when you otherwise would be only a few characters over the limit. In fairness a line that approaches 119 would probably be best split up anyway, but it's our choice if it's really the best option, not of the style checker.

To be honest, I don't really know. It's something I have picked up somewhere over time and it seems to work great. I guess an advantage would be that with only 72 characters not to many from imports are on one line, see test_piplicenses.py for example.

Okay, I understand. Indeed, it seems to be working well.

That doesn't mean that every line should now be longer, but that in some limited cases it improves readability when you otherwise would be only a few characters over the limit.

In limited cases, you're right. I'm following the Python community standard for now.

raimon49 · 2021-01-20T12:15:05Z

After this PR is finished, I would like to propose moving from a single source file to a package structure. piplicenses.py is already quite large with almost 900 lines and it would only continue to grow in the future. The move could improve the readability. What do you think?

Yes, I agree with your proposal.

We have a lot of unit test code. It will also help when changing to a package structure. The test code you added is one of them.

raimon49

I checked this P-R.

It will be imported and shipped in the next release version 3.3.0. The work is scheduled for the weekend around 2021-01-23.

cdce8p · 2021-01-20T13:40:24Z

Thanks for the quick review.
If it's up to me, you don't need to release another version just yet. I would rather continue with some other ideas, like the aforementioned move to a package structure.

Would it be an idea to create a separate dev branch and merge the changes against that one? Most big projects I know do it that way. For a release you would then merge dev into master and, to avoid conflicts, master back into dev. That seems to work quite well.

cdce8p · 2021-01-20T14:12:56Z

That's also where squash merge has the most benefits.

Just to clarify, how the whole process would look like:

Squash merge every new PR into dev
For new release
2.1. Push new commit updating the version string
2.2. Create new PR, dev against master, this will run all CI on last time
2.3. If CI passes, normal merge PR into master
2.4. Since master now contains an additional merge commit that dev doesn't have, merge master back into dev. A normal push is enough
Go back to step 1

Some additional notes:

If you set the default branch to dev, all new PRs will target it by default
All changes, except version updates, should only be squash merged via a PR. This guarantees that all commits which show up in git blame reference a PR with additional information
If all changes have a PR, it also makes sure that all checks have passes and you don't accidentally committed something which causes a bug.
You could add the requirement that all checks must pass before a PR can be merged
Optional add that every PR needs at least one approving review
I believe it's also possible to set branch protections in Github, eg. for master don't allow push commits

--
Please let me know if you have any questions

raimon49 · 2021-01-20T15:04:18Z

To express my personal opinion:

I am against having a permanent dev branch. You are right, it is profitable for big projects to develop several new features in parallel. But I do not see this tool as a big project. It is better to keep the flow simple, as shown in the GitHub flow.

Next, let's talk about the release plan.

Improve argparser is a simple and beautiful advantage for many users.

On the other hand, in the process of embarking on the move to the package structure, it is possible that some of the most recent issues that have arrived can be resolved. (e. g. #82 #81 )

It seems to me that this effort is worth working on the base branch as you suggest. For example, create a base branch such as dev-4.0.0 or release-4.0.0. However, as I mentioned earlier, I am against a permanent branch. So, when the goal of the package structure is achieved and shipped, this development branch will end its lifecycle.

I'm waiting for your opinion.

cdce8p · 2021-01-20T16:08:48Z

That works for me. My intention behind the suggestion was simply to allow for a faster development pace. Implementing all changes would otherwise either require one huge PR, which would basically be unreviewable, or take months to complete. Having a separate branch thus allows for a faster turnaround once you approve a PR. I don't really have a name preference.

Regarding the issues you mentioned, it's definitely worth keeping them in mind. I also mentioned some other topics in my last PR #86 which I would like to work on. Targeting 4.0 would certainly make things easier, regarding breaking changes.
I plan on opening a new issue later to track all topics for an upcoming release. This would also be a good place to have general discussions if topics pop up.

cdce8p · 2021-01-22T13:08:38Z

While I was working on #88, I noticed some small issues. The last commit address these.

raimon49 · 2021-01-22T14:31:17Z

@cdce8p Am I correct in understanding that your work on the improve-argparser branch is complete?

Then I will start the next task. Please let me know if there is any miscommunication.

Merge this P-R (with squash)
Version 3.3.0 will be shipped based on the master branch.
Create a remote branch dev-4.0.0 .

cdce8p · 2021-01-22T14:39:08Z

@raimon49 That is correct. This PR is complete.

Co-authored-by: cdce8p <30130371+cdce8p@users.noreply.github.com> Squashed commit of the following: commit b7949b5 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Fri Jan 22 14:04:52 2021 +0100 Small bugfixes commit 8441671 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Tue Jan 19 20:44:22 2021 +0100 Update gitignore commit 53ebae0 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Tue Jan 19 20:00:13 2021 +0100 Add type hints for args commit ebc95e2 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Tue Jan 19 19:52:04 2021 +0100 Add isort config + reorder imports commit 3f08e86 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Tue Jan 19 19:50:15 2021 +0100 Add arg verify commit 2b885a1 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Tue Jan 19 00:39:06 2021 +0100 Bugfix display default values commit 72ddf16 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 23:54:10 2021 +0100 Use enums for arguments (from, order, format) commit 99ec862 Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 17:09:50 2021 +0100 Update readme commit 44cdf4e Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 16:48:56 2021 +0100 Add custom help text formatter commit 52055be Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 16:41:02 2021 +0100 Add argument groups commit 78b96bb Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 16:38:23 2021 +0100 Reorder arguments commit 0af008b Author: cdce8p <30130371+cdce8p@users.noreply.github.com> Date: Mon Jan 18 16:28:14 2021 +0100 Change code formatting - argparser * No code changes!

cdce8p · 2021-01-23T09:51:40Z

@raimon49 Something went wrong with the squash merge. Did you do it with Github?
To be honest, I would like to see it fixed. I could post some commands that should do it if you agree.

cdce8p · 2021-01-23T10:27:44Z

The following should work:

Disable branch protection for master (if enabled)
Local git commands

git fetch origin
git checkout master
git rebase origin/master
git checkout -b master-old
git checkout master
git reset --hard HEAD~2
git push -f

Reopen Improve argparser #87 + from Github select and do squash merge (into master)
Local git commands

git fetch origin master
git rebase origin/master
git cherry-pick c2da089e63b47121cddf73dcc85695a29bb14570  # Bump version
git cherry-pick 3634bab33900f30e2863a5256f906c24289937cd  # isort
git cherry-pick 4ace27c31978f42f77b979f90549277f94986ac3  # make target 'format'
git cherry-pick -m 1 a5cc189f88f50095533d98d0ec8230229b9d2ca2   # merge PR #90
git commit --allow-empty
git push

git checkout dev-4.0.0
git reset --hard master
git push -f

git branch -D master-old

Reenable branch protection for master

raimon49 · 2021-01-23T11:25:01Z

@cdce8p I worked on it.

cdce8p · 2021-01-23T11:32:28Z

Looks like it worked. Thanks!

cdce8p · 2021-01-23T11:42:40Z

@raimon49 Just one last thing: Do you want to release this as 3.3.1? That way we could guarantee this issue shouldn't pop up anywhere in the future.

Edit: Nevermind, just saw that you updated the release as well. That should be enough.

raimon49 · 2021-01-23T12:48:14Z

I have released 3.3.0 to PyPI based on commit 4ace27c.

As far as I can see, there is no difference from the master branch that was forced to be pushed.

$ git diff origin/master 4ace27c31978f42f77b979f90549277f94986ac3

I have determined that users who have the package installed will not be affected. So I decide that there is no need to release 3.3.1.

cdce8p added 7 commits January 18, 2021 16:35

Change code formatting - argparser

0af008b

* No code changes!

Reorder arguments

78b96bb

Add argument groups

52055be

Add custom help text formatter

44cdf4e

Update readme

99ec862

Use enums for arguments (from, order, format)

72ddf16

Bugfix display default values

2b885a1

raimon49 reviewed Jan 19, 2021

View reviewed changes

cdce8p added 3 commits January 19, 2021 19:50

Add arg verify

3f08e86

Add isort config + reorder imports

ebc95e2

Add type hints for args

53ebae0

cdce8p force-pushed the improve-argparser branch from 6b6d09e to 53ebae0 Compare January 19, 2021 19:03

Update gitignore

8441671

raimon49 reviewed Jan 20, 2021

View reviewed changes

raimon49 approved these changes Jan 20, 2021

View reviewed changes

This was referenced Jan 21, 2021

Move to package structure #88

Merged

(Frozen release plans) Progress to 4.0 #89

Open

Small bugfixes

b7949b5

raimon49 mentioned this pull request Jan 23, 2021

Release 3.3.0 #90

Merged

5 tasks

raimon49 closed this in #90 Jan 23, 2021

raimon49 reopened this Jan 23, 2021

raimon49 merged commit 5bae7b5 into raimon49:master Jan 23, 2021

cdce8p deleted the improve-argparser branch January 23, 2021 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve argparser #87

Improve argparser #87

cdce8p commented Jan 18, 2021 •

edited

Loading

codecov bot commented Jan 18, 2021 •

edited

Loading

raimon49 commented Jan 19, 2021

raimon49 Jan 19, 2021

cdce8p Jan 19, 2021

cdce8p commented Jan 19, 2021

cdce8p commented Jan 19, 2021

raimon49 Jan 20, 2021

cdce8p Jan 20, 2021

raimon49 Jan 20, 2021

raimon49 commented Jan 20, 2021

raimon49 left a comment

cdce8p commented Jan 20, 2021

cdce8p commented Jan 20, 2021

raimon49 commented Jan 20, 2021

cdce8p commented Jan 20, 2021

cdce8p commented Jan 22, 2021

raimon49 commented Jan 22, 2021

cdce8p commented Jan 22, 2021

cdce8p commented Jan 23, 2021

cdce8p commented Jan 23, 2021

raimon49 commented Jan 23, 2021

cdce8p commented Jan 23, 2021

cdce8p commented Jan 23, 2021 •

edited

Loading

raimon49 commented Jan 23, 2021

Improve argparser #87

Improve argparser #87

Conversation

cdce8p commented Jan 18, 2021 • edited Loading

codecov bot commented Jan 18, 2021 • edited Loading

Codecov Report

raimon49 commented Jan 19, 2021

raimon49 Jan 19, 2021

Choose a reason for hiding this comment

cdce8p Jan 19, 2021

Choose a reason for hiding this comment

cdce8p commented Jan 19, 2021

cdce8p commented Jan 19, 2021

raimon49 Jan 20, 2021

Choose a reason for hiding this comment

cdce8p Jan 20, 2021

Choose a reason for hiding this comment

raimon49 Jan 20, 2021

Choose a reason for hiding this comment

raimon49 commented Jan 20, 2021

raimon49 left a comment

Choose a reason for hiding this comment

cdce8p commented Jan 20, 2021

cdce8p commented Jan 20, 2021

raimon49 commented Jan 20, 2021

cdce8p commented Jan 20, 2021

cdce8p commented Jan 22, 2021

raimon49 commented Jan 22, 2021

cdce8p commented Jan 22, 2021

cdce8p commented Jan 23, 2021

cdce8p commented Jan 23, 2021

raimon49 commented Jan 23, 2021

cdce8p commented Jan 23, 2021

cdce8p commented Jan 23, 2021 • edited Loading

raimon49 commented Jan 23, 2021

cdce8p commented Jan 18, 2021 •

edited

Loading

codecov bot commented Jan 18, 2021 •

edited

Loading

cdce8p commented Jan 23, 2021 •

edited

Loading