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

freetype: format #133165

Merged
merged 2 commits into from
Aug 16, 2021
Merged

freetype: format #133165

merged 2 commits into from
Aug 16, 2021

Conversation

SuperSandro2000
Copy link
Member

Motivation for this change
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 packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@yu-re-ka yu-re-ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait for some discussion to happen regarding formatting of packages before merging more of these PRs

@SuperSandro2000
Copy link
Member Author

Why? This is the most standard formatting that is described in almost all examples and formatting has been going on for a few months and so far no one had any problem with it.

@yu-re-ka
Copy link
Contributor

To just reiterate the technical aspects I already brought up in #133360:

  • It breaks git blame. For example if I wanted to see when these build flags were last changed:
    screenshot1
    Right now I can still go back one commit and look at the blame before the formatting, but once there are new changes in the file, I will not get the full picture.
  • It breaks git revert, git merge and the likes for commits before the formatting. For example if I wanted to revert the last nss update:
    screenshot1
    Gosh, now I have to resolve a conflict first.

So from my point of view this brings many annoyances and not a lot of advantages. A less invasive way like requiring a well-defined formatting on all newly changed lines would seem much more appropriate to me.

Of course there will be some people like you who think this should be done, and others like me who think it shouldn't be done. But this is why we should discuss it first instead of "just doing" it.

@SuperSandro2000
Copy link
Member Author

It breaks git blame

git blame --ignore-rev. Not my fault that GitHubs UI does not expose those features.

For example if I wanted to revert the last nss update:

If there was a really urgent pressing issue to revert the nss update then just also revert the formatting commit but I don't think that will happen. Firefox requires the latest version most of the time and that won't change in the future. Reverting an update is always the last option and is normally not done.

Of course there will be some people like you who think this should be done, and others like me who think it shouldn't be done. But this is why we should discuss it first instead of "just doing" it.

Nothing happened the last months and I don't believe this discussion will move anywhere in the next months. This is just stalling.


It is far more work to tell people that the code they copied from some (core) library is outdated and does not follow current coding standard at all.

@yayayayaka
Copy link
Member

yayayayaka commented Aug 11, 2021

It breaks git blame

git blame --ignore-rev. Not my fault that GitHubs UI does not expose those features.

Please also consider that it adds a burden to newer contributors who might not be an expert in version control yet and do not know all the git manpages by heart.

@yu-re-ka
Copy link
Contributor

git blame --ignore-rev

I did not know about this flag. Thanks!

I don't believe this discussion will move anywhere in the next months. This is just stalling.

My intention is not to stall the formatting efforts, but to make everyone think about if this is actually the best way to achieve better formatting and also consider disadvantages of this approach.

Quite frankly I am also a bit annoyed that you merged #133360 regardless of the concerns I mentioned. While we have almost no rules, there is a certain contributor etiquette, and not merging one's own PRs (and even more so when there are open concerns) is part of it.

One way to get rid of all my doubts would be to create an RFC on the topic of improving formatting in nixpkgs.

@SuperSandro2000
Copy link
Member Author

One way to get rid of all my doubts would be to create an RFC on the topic of improving formatting in nixpkgs.

That just ends up in an endless discussion that goes nowhere and I frankly do not have the time for that.

@veprbl
Copy link
Member

veprbl commented Aug 16, 2021

It should be fine to merge this PR, as it looks fine by itself. We can still discuss formatting strategies do's and dont's, if need be. Do we agree on that?

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Aug 16, 2021

It should be fine to merge this PR, as it looks fine by itself. We can still discuss formatting strategies do's and dont's, if need be. Do we agree on that?

I don't see any problem that is specific to this PR either, but still think this kind of PRs should not be be done on a larger scale without further discussion and planning.

So no, I don't think it's fine to continue creating these PRs in mass and merging them faster than one can look (as it's still happening).

At least that's my opinion.

@veprbl veprbl merged commit b3b61f7 into master Aug 16, 2021
@veprbl
Copy link
Member

veprbl commented Aug 16, 2021

I don't see any problem that is specific to this PR either, but still think this kind of PRs should not be be done on a larger scale without further discussion and planning.

Different collaborators will have different priorities and may come up with changes that may appear spontaneous to you, just as you may come up with comments that will appear spontaneous to them. Predictability is good, but let's have reasonable expectations about how much of it is can usually be achieved.

@veprbl veprbl deleted the freetype branch August 16, 2021 19:07
@jonringer
Copy link
Contributor

I was supposed to do a tree-wide nixfmt before branching off 21.05, but forgot when it came to do it.

we can postpone doing formatting PRs for ~3 months, and just deal with the pain once for the 21.11 branch off

cc @SuperSandro2000 @domenkozar @vcunat

@vcunat
Copy link
Member

vcunat commented Aug 20, 2021

I assume the discussion is moving to the RFC: NixOS/rfcs#101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants