python3Packages: format with nixfmt #313628
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
6.topic: python
6.topic: policy discussion
6.topic: jupyter
|
Result of 4 packages built:
|
|
The @NixOS/nix-formatting team wasn't planning on doing any large reformats just yet, because the formatter isn't stable yet, but it is indeed a good time for it. The team will try to make a decision on this in time and merge it if ready. |
|
🤔 diff --git a/pkgs/development/python-modules/waitress-django/default.nix b/pkgs/development/python-modules/waitress-django/default.nix
index fad82f4952ce..925f37df7487 100644
--- a/pkgs/development/python-modules/waitress-django/default.nix
+++ b/pkgs/development/python-modules/waitress-django/default.nix
@@ -1,4 +1,9 @@
-{ lib, buildPythonPackage, django, waitress }:
+{
+ lib,
+ buildPythonPackage,
+ django,
+ waitress,
+}:
buildPythonPackage {
pname = "waitress-django";
@@ -6,7 +11,10 @@ buildPythonPackage {
format = "setuptools";
src = ./.;
- pythonPath = [ django waitress ];
+ pythonPath = [
+ django
+ waitress
+ ];
doCheck = false;
meta = with lib; {- /nix/store/vg9g8l1k4az9n4hrwr3hxcpslydsbl61-python3.11-waitress-django-1.0.0.drv:{out}
+ /nix/store/yx5ig3lhizrx95ag7yvsn8p2cfv4gyy8-python3.11-waitress-django-1.0.0.drv:{out}
• The input source named `waitress-django` differs
• The environments do not match:
src=''
-/nix/store/cc95i27n77zij2m52246ylgf9fakm18f-waitress-django
+/nix/store/pyn8sxz49a939a75mm78bgghl9dmjmsr-waitress-django
'So very cool, that we have the whole input in nixpkgs. |
|
The I don't think this is a relevant blocker. |
|
Eval looks fine, going to push the |
37 tasks
mweinelt
requested review from
ajs124,
lukegb,
Mic92,
zowoq and
a team
as code owners
infinisil
reviewed
May 22, 2024
| @@ -46,6 +46,7 @@ jobs: | |||
| NIX_FMT_PATHS_VSCODE_EXTS: pkgs/applications/editors/vscode/extensions | |||
| NIX_FMT_PATHS_PHP_PACKAGES: pkgs/development/php-packages | |||
| NIX_FMT_PATHS_BUILD_SUPPORT_PHP: pkgs/build-support/php | |||
| NIX_FMT_PATHS_PYTHON_PACKAGES: pkgs/development/python-modules | |||
There was a problem hiding this comment.
This is a little bit problematic for directories that frequently get PRs with new files, because those files are not formatted properly yet and would break master when merged
There was a problem hiding this comment.
I agree that the overall situation is unfortunate, but maybe this is less bad than having people reformat every file individually as they touch it?
There was a problem hiding this comment.
What I understand from this is:
- The CI action does not check new files in a PR
- The CI action will notice wrong formatting once merged into master, and subsequentl fail
Is that anything that can be sorted out in a reasonable timeframe? While Python is a very busy package set, other consumers of the CI action are surely dealing with that issue as well.
There was a problem hiding this comment.
I'd be fine with deferring the CI action for now, but going for the treewide reformat. I think that would get rid of most of the churn and your concerns about the CI action.
|
A concrete but minor problem is that because NixOS/nixfmt#201 (and potentially more like that) isn't merged yet (and more cases could exist), not all empty lines are preserved as the RFC specified. So formatting with the current version "loses" a bit of information |
|
That kind of information loss doesn't sound too concerning to me. What I'm more concerned is, is the amount of churn. Have you seen the number of conflicting files? So the reformat is actually already happening. |
piegamesde
approved these changes
May 22, 2024
13 tasks
13 tasks
|
For the record: Changes like this should be scheduled in advance, especially during ZHF. Now, we have probably over a 100 merge conflicts in open PRs (not all ZHF-related, though) on our hands which could have been easily avoided. |
|
All of this could easily have been avoided if people could stand still for a bit and await coordination, instead of starting to drive-by format files one by one. I take co-responsibility for these merge conflicts, and I remain convinced that doing this was the less bad option. |
13 tasks
|
@piegamesde In our team processes we document that non-trivial decisions need to be done by at least two team members, please honor that. And indeed the RFC documents that formatting should happen before the branch-off, this wasn't honored either (Edit: it was, my fault). It's definitely not great to have this much churn though, I wasn't aware of this, I wish the formatting team was pinged more for that. |
Why are you saying this wasn't honored? It was merged 4 minutes before the start of the branch-off meeting. I was even clearing it up with Weija on the call. |
|
Ah sorry! I wasn't aware of that, and didn't properly check it myself (just compared the time of the branch-off announcement against the merge time of this PR). I guess it doesn't make sense for this to be reverted at this point. Personally I don't have a lot of time to help with that now, I was expecting this to be done only at the next release. |
|
The strategy to rebase conflicting changes is:
Running the reformat first will reduce the chance for conflicts significantly. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-05-28/46126/1 |
13 tasks
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
imincik
added a commit
to imincik/geospatial-nix
that referenced
this pull request
Jun 6, 2024
This was referenced Jul 3, 2024
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
People have started reformatting individual packages with nixfmt, so a treewide change should get rid of that review churn.
Also, we're at an ideal point to sneak in a reformat, when we consider backports from master to release.
If this causes rebuilds, we'll need to identify those and postpone.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.