Skip to content
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

python3Packages.python-gitlab: 1.15.0 -> 2.2.0 #86269

Merged
merged 2 commits into from May 4, 2020

Conversation

@das-g
Copy link
Member

das-g commented Apr 28, 2020

Motivation for this change

upstream releases (see PyPI for SHA256 hashes & GitHub for release notes)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Result of nixpkgs-review pr 86269 1

2 packages built:
- python37Packages.python-gitlab
- python38Packages.python-gitlab
Copy link
Contributor

jonringer left a comment

Please follow CONTRIBUTING.md and manual#submitting-changes-making-patches and squash the commits.

This is a lot of small bumps, I would just compile into one

python3Packages.python-gitlab: 1.15.0 -> 2.2.0
@das-g
Copy link
Member Author

das-g commented May 2, 2020

Thanks for the feedback!

Please follow CONTRIBUTING.md [...]

What part of CONTRIBUTING.md did I not follow? (Honest question. I just read it again and can't determine where I went afoul of it.)

[...] and manual#submitting-changes-making-patches and squash the commits.

This is a lot of small bumps, I would just compile into one

python3Packages.python-gitlab: 1.15.0 -> 2.2.0

Does that manual section mandate squashing of all commits? The only rule towards that direction seems to be

  • If you have commits pkg-name: oh, forgot to insert whitespace: squash commits in this case. Use git rebase -i.

None of my commits here are of "oh, forgot to insert whitespace" nature, as far as I can tell.

That manual section also says:

  • Make commits of logical units.

That's what I tried to do here. Note that I didn't make separate commits for all versions since 1.15.0. My reasons for the commits are as follows:

  • be60e99 re-format with nixfmt: Unrelated to the version bumps. Would result in confusing / misleading change reasons being shown by git blame if it'd be included in a version bump commit.
  • a27308c 1.15.0 -> 2.0.0: Major release, this bump is the reason for dropping six and for disabling on Python < 3.6
  • 7cceb71 2.0.0 -> 2.0.1: Latest patch release of 2.0 minor release group
  • 548bb89 2.0.1 -> 2.1.2: Latest patch release of 2.1 minor release group (Note that I skipped over 2.1.0 and 2.1.1)
  • a770650 2.1.2 -> 2.2.0: (Current) latest patch release of 2.2 minor release group

Yes, these are many bumps, but I think having them in the history is worthwhile. If you maintain that they are too many, I'd like to at least preserve be60e99 & a27308c (and squash 7cceb71, 548bb89 and a770650 to one single commit, resulting in a total of 3 commits), unless you insist they be all squashed into only one commit.

@das-g das-g requested a review from jonringer May 2, 2020
@das-g
Copy link
Member Author

das-g commented May 2, 2020

Does 3 commits sound good to you?

Either

or

make most sense, if not all 5 commits shall be preserved.

@jonringer
Copy link
Contributor

jonringer commented May 3, 2020

From the perspective of the master branch, I think it makes more sense to just do all the bumps in one go, because the individual package bumps never existed. But that's just me

@das-g das-g force-pushed the das-g:bump-python-gitlab branch from a770650 to 05d6ef2 May 3, 2020
@das-g
Copy link
Member Author

das-g commented May 3, 2020

From the perspective of the master branch, I think it makes more sense to just do all the bumps in one go, because the individual package bumps never existed.

Alright, I've squashed the four version bump commits into one, but left the re-format commit as-is.

But that's just me

Not quite sure what to make of that. If you require changes for an approving review, please give actionable feedback. If however the PR just isn't to your best liking, but you find it objectively acceptable, please state that clearly.

@jonringer
Copy link
Contributor

jonringer commented May 4, 2020

Not quite sure what to make of that. If you require changes for an approving review, please give actionable feedback. If however the PR just isn't to your best liking, but you find it objectively acceptable, please state that clearly.

To me, you're doing a version bump, if you have a series of 4 version bumps, why not collapse it into a single version bump? If there was a regression in this package, then having the many version bumps doesn't really help, because you are trying to triage regressions from master. So having an intermediate commit, which never had a release on that package, seems to add noise when doing a git bisect.

Copy link
Contributor

jonringer left a comment

LGTM

Result of nixpkgs-review pr 86269 1

2 packages built:
- python37Packages.python-gitlab
- python38Packages.python-gitlab

@jonringer jonringer merged commit de9f8c3 into NixOS:master May 4, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="05d6ef2"; rev="05d6ef205b2fbe6a4e0813c82aaaeb85f0ae610e"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.