cdce8p commented 3 years ago

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).

codecov[bot] commented 3 years ago

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%)`	:arrow_up:

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 commented 3 years ago

@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.

cdce8p commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

cdce8p commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

raimon49 commented 3 years ago

@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 commented 3 years ago

@raimon49 That is correct. This PR is complete.

cdce8p commented 3 years ago

@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 commented 3 years ago

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 #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


5. Reenable branch protection for `master`

raimon49 commented 3 years ago

@cdce8p I worked on it.

cdce8p commented 3 years ago

Looks like it worked. Thanks!

cdce8p commented 3 years ago

@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 commented 3 years ago

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

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.

raimon49 / pip-licenses

Improve argparser #87

Codecov Report