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.

Sign up for GitHub

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

Jump to bottom

bat: add meta.mainProgram #246943

Merged
merged 1 commit into from
Aug 3, 2023
Merged

bat: add meta.mainProgram #246943

merged 1 commit into from
Aug 3, 2023

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Aug 3, 2023

Related PR:

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@drupol drupol marked this pull request as ready for review August 3, 2023 09:49
@zowoq
Copy link
Contributor

zowoq commented Aug 3, 2023

bat: add meta.mainProgram

What are all the unrelated changes?

@drupol
Copy link
Contributor Author

drupol commented Aug 3, 2023

bat: add meta.mainProgram

What are all the unrelated changes?

Just a minor cleanup, nothing fancy.

  • Using makeBinaryWrapper because it doesn't create issues on Darwin platform
  • Remove the prepending with lib; because this is somehow controversial

@zowoq
Copy link
Contributor

zowoq commented Aug 3, 2023

Remove the prepending with lib; because this is somehow controversial

Why?

@drupol
Copy link
Contributor Author

drupol commented Aug 3, 2023

There are several compelling reasons to avoid using a with statement.

Firstly, employing such a statement imports all the functions from lib into your current scope. This can potentially cloud your code's clarity and lead to confusion. A more preferable alternative would be using a targeted statement like inherit (lib) abc def; which enhances the readability of your code. This way, you can accurately trace the origin of each function.

Secondly, a with statement could interfere with your Language Server Protocol (LSP). Given that the entire lib scope is imported, your LSP might struggle to pinpoint the sources of various functions, resulting in a poor development experience.

There are additional considerations regarding this topic that fall outside my expertise, but I encourage you to post this question on discourse for a more comprehensive discussion.

You can find a very good article about that on nix.dev.

There is also this RFC: https://github.com/r-burns/nixos-rfcs/blob/rfc-inherit-as-list/rfcs/0110-inherit-as-list.md

@zowoq
Copy link
Contributor

zowoq commented Aug 3, 2023 •
edited
Loading

Unless it's in the nixpkgs docs it is stylistic preference, please revert it.

@drupol
Copy link
Contributor Author

drupol commented Aug 3, 2023

Closing the PR.

@drupol drupol closed this Aug 3, 2023
@drupol drupol deleted the bat/add-meta-mainProgram branch August 3, 2023 10:04
@drupol
Copy link
Contributor Author

drupol commented Aug 3, 2023

Oops, wrong window.

@drupol drupol restored the bat/add-meta-mainProgram branch August 3, 2023 10:04
@drupol drupol reopened this Aug 3, 2023
@yu-re-ka
Copy link
Contributor

yu-re-ka commented Aug 3, 2023 •
edited
Loading

Global with lib; is bad. local with lib; in front of one section that makes a lot of uses of this attrset is fine by all accounts I have heard

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Aug 3, 2023

For me it is fine if further cleanup changes happen in this PR, however please put them in separate commits that do not have the unsuspecting commit message "add meta.mainProgram"

@drupol
Copy link
Contributor Author

drupol commented Aug 3, 2023

I'm not going to try to argue here, for me with ...; should not exists. I might be extreme, but I found it extremelly confusing everytime I see it.

Global with lib; is bad. local with lib; in front of one section that makes a lot of uses of this attrset is fine by all accounts I have heard

Just a remark on this, depending on the contextual scope you're looking at it, a with lib; statement can be considered as global.

But ok, I don't want to loose too much time on this, I've reverted the changes, please let's move on.

yu-re-ka
yu-re-ka reviewed Aug 3, 2023
View reviewed changes
pkgs/tools/misc/bat/default.nix
@@ -16,7 +16,7 @@ rustPlatform.buildRustPackage rec {

src = fetchFromGitHub {
owner = "sharkdp";
repo = pname;
repo = "bat";
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific reason for this change? repo = pname seems to be a very common pattern

Copy link
Contributor Author

@drupol drupol Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

repo = pname; is nice for DRY but creates a binding that goes too far.

See further information about this here: nix-community/nixpkgs-lint#21

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK finalattrs hasn't been implemented for buildRustPackage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly not yet indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So there isn't a reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is still valid, even without finalAttrs pattern. If one needs to overrides pname, it will also have an impact on the src. This change is preventing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't ask you to revert it but frankly this change is pointless.

I don't want to loose too much time on this

I'd ask you to consider that other people may not want to loose too much time on stuff like this either.

This PR could have been a one line diff adding mainProgram that could have been merged without discussion as soon as it passed ofborg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm dedicating my time for free for this project and every time I edit a PR, I'm trying to use what the "seniors" has been telling me for months when I was submitting my first PRs.

Constantly refining for a greater good the things that I'm "touching", I see that it is not very welcome, point noted, maybe I should avoid dedicating time to this community?

@zowoq zowoq merged commit bff3b3c into master Aug 3, 2023
10 checks passed
@zowoq zowoq deleted the bat/add-meta-mainProgram branch August 3, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zowoq zowoq zowoq left review comments

@yu-re-ka yu-re-ka yu-re-ka left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drupol @zowoq @yu-re-ka