Skip to content

Revert (parts of) "Treewide cleanup meta description" #320283

Closed
Aleksanaa wants to merge 2 commits intoNixOS:masterfrom
Aleksanaa:prefixing-article
Closed

Revert (parts of) "Treewide cleanup meta description" #320283
Aleksanaa wants to merge 2 commits intoNixOS:masterfrom
Aleksanaa:prefixing-article

Conversation

@Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Jun 16, 2024

Description of changes

This reverts #317959

That PR does not clearly indicate that it adds a new rule, and does not put the modifications to the document in a separate commit, thus avoiding the discussion of adding a new rule. Such rules obviously deserve more discussion.

Add recommendation to avoid meta.description to start with the definite or an indefinite article to pkgs/README.md
Remove the definite or an indefinite article from the beginning of meta.description

Personally, I don't think this modification makes any sense, and often makes reading descriptions less intuitive. Some people also noted that this is not grammatically appropriate, or is unnecessarily too strict.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: pantheon The Pantheon desktop environment 6.topic: cinnamon Desktop environment 6.topic: agda A dependently typed programming language / interactive theorem prover 6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: zig Zig is an imperative, general-purpose, statically typed, compiled system programming language. 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php PHP is a general-purpose scripting language geared towards web development. 6.topic: k3s Kubernates distribution (https://k3s.io/) 6.topic: dotnet Language: .NET labels Jun 16, 2024
With manual solving conflicts:
pkgs/applications/audio/freac/default.nix
pkgs/by-name/ra/rav1e/package.nix
pkgs/by-name/wh/where-is-my-sddm-theme/package.nix

This reverts NixOS#317959

That PR does not clearly indicate that it adds a new rule, and does not
put the modifications to the document in a separate commit, thus
avoiding the discussion of adding a new rule. Such rules obviously
deserve more discussion.

Personally, I don't think this modification makes any sense, and often
makes reading descriptions less intuitive. Some people also noted that
this is not grammatically appropriate, or is unnecessarily too strict.
With manual solving conflicts:
pkgs/applications/audio/moc/default.nix
pkgs/applications/audio/ncpamixer/default.nix
pkgs/networking/cluster/containerpilot/default.nix
pkgs/by-name/al/allmark/package.nix
pkgs/by-name/gr/gruvbox-gtk-theme/package.nix
pkgs/servers/http/hiawatha
pkgs/tools/filesystems/tmsu

This reverts NixOS#317959

That PR does not clearly indicate that it adds a new rule, and does not
put the modifications to the document in a separate commit, thus
avoiding the discussion of adding a new rule. Such rules obviously
deserve more discussion.

Personally, I don't think this modification makes any sense, and often
makes reading descriptions less intuitive. Some people also noted that
this is not grammatically appropriate, or is unnecessarily too strict.
@Aleksanaa Aleksanaa force-pushed the prefixing-article branch from e3e76ff to be41b89 Compare June 16, 2024 15:08
@marcusramberg
Copy link
Contributor

I think the more succinct descriptions are an improvement for the wast majority of cases, so I'm opposed to this revert.

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jun 16, 2024

I agree that sneaking in a rule is not ideal, but indefinite articles are quite redundant IMO. Not sure why it'd be "grammatically incorrect" either, as it's not a full sentence, merely a fragment.

In any case, the only thing sillier than adding this rule is reverting it IMO

@Aleksanaa
Copy link
Member Author

In any case, the only thing sillier than adding this rule is reverting it IMO

It doesn't seem to matter whether it's grammatically correct or not, but I'm curious, does this mean we're on a good start for more of these modifications that don't need to be discussed? Have we given voice to those who want to speak?

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jun 16, 2024

It's a low-stakes change, not like (say) enforcing a new rule about maintainership or such. Talking about "giving voice to those who want to speak" sounds quite dramatic for this specific scenario.

Also it's up to reviewers to catch these changes as well, if it's that important.

@Aleksanaa
Copy link
Member Author

It's a low-stakes change, not like (say) enforcing a new rule about maintainership or such.

Some reviewers have previously requested that articles be added. Some distributions enforce the article and some don't. You have also found that after making such changes, it becomes more difficult to undo.

Talking about "giving voice to those who want to speak" sounds quite dramatic for this specific scenario.
Also it's up to reviewers to catch these changes as well, if it's that important.

This is merged faster than most "add new package" PRs I've seen. And it merges even faster than parts of my simple upgrades by-hand. Such speed itself is inappropriate. This means that, before the merge, its visibility did not match the scope of its changes.

@ocfox
Copy link
Member

ocfox commented Jun 16, 2024

This is merged faster than most "add new package" PRs I've seen. And it merges even faster than parts of my simple upgrades by-hand.

Agree that but it is not the reason to revert. #317959 is a good change for me, if a revert is needed we could discuss under that.

@adamcstephens
Copy link
Contributor

Without a linter these rules will only be weakly enforced, even if widely desirable. Inevitably they will be violated as nixpkgs progresses.

@K900
Copy link
Contributor

K900 commented Jun 16, 2024

Can we not have a policy conversation on a PR that has more than a screen's height of reviewers?

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jun 16, 2024

Some reviewers have previously requested that articles be added. Some distributions enforce the article and some don't.

And I've been nitted in both directions; that's the strongest indicator to just add a rule to stop the nits. At least this direction has precedent from other distros and has some logic.

This is merged faster than most "add new package" PRs I've seen.

I can't argue with that, I guess, since some package inits (even those in good shape) have never been merged, so merging at all is faster than never. I can only assume that since 5 different committers commented (which is also quite rare, usually everyone ignores treewides for weeks) and dozens of reviewers were tagged, but no one raised an objection, it was considered uncontroversial.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 16, 2024
@Aleksanaa
Copy link
Member Author

I don't think this PR will lead to any meaningful conclusion.

@Aleksanaa Aleksanaa closed this Jun 16, 2024
@NixOS NixOS locked and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

6.topic: agda A dependently typed programming language / interactive theorem prover 6.topic: cinnamon Desktop environment 6.topic: dotnet Language: .NET 6.topic: emacs Text editor 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: erlang General-purpose, concurrent, functional high-level programming language 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: k3s Kubernates distribution (https://k3s.io/) 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: pantheon The Pantheon desktop environment 6.topic: php PHP is a general-purpose scripting language geared towards web development. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: vim Advanced text editor 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: xfce The Xfce Desktop Environment 6.topic: zig Zig is an imperative, general-purpose, statically typed, compiled system programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants