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

Avoid top-level with ...; in pkgs/development/lisp* #299241

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Avoid top-level with ...; in pkgs/development/lisp* #299241

wants to merge 2 commits into from

Conversation

philiptaron
Copy link
Contributor

Description of changes

Using with at a top-level scope is an anti-pattern. The tracking issue is #208242. This is a pure refactor: there is no functional change contained in this PR.

I tried out the other strategy of replacing top-level with in this PR. If symbol foo was pulled implicitly from attrset pkgs, I replaced foo with pkgs.foo. Prior to this change, this file was a mix of both styles (foo and pkgs.foo), and now it's just one.

There are no rebuilds when I run nixpkgs-review rev HEAD, so I've targeted master with this PR.

Things done

  • Tested compilation of all packages that depend on this change using nixpkgs-review rev HEAD.
  • Fits CONTRIBUTING.md.

@philiptaron philiptaron mentioned this pull request Mar 26, 2024
Closed
6 tasks
@philiptaron philiptaron force-pushed the issue-208242/lisp-modules-part-1 branch from 435f217 to 0589480 Compare March 26, 2024 19:29
@philiptaron philiptaron force-pushed the issue-208242/lisp-modules-part-1 branch from 0589480 to 60ebb1a Compare March 26, 2024 20:13
fricklerhandwerk
fricklerhandwerk reviewed Mar 28, 2024
View reviewed changes
pkgs/development/lisp-modules-new-obsolete/ql.nix

# FIXME: automatically add nativeLibs based on conditions signalled

# Try to keep this list sorted
extras = {
cffi-libffi = pkg: {
nativeBuildInputs = [ libffi ];
nativeLibs = [ libffi ];
nativeBuildInputs = [ pkgs.libffi ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'd have done extras = with pkgs; { instead. Not sure this is worth the diff, as it's also adding noise now.

philiptaron
philiptaron commented Mar 28, 2024
View reviewed changes
pkgs/development/lisp-modules-new-obsolete/ql.nix
Comment on lines 47 to 48
nativeBuildInputs = [ pkgs.libdevil ];
nativeLibs = [ pkgs.libdevil ];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These lines are doing pkgs.

philiptaron
philiptaron commented Mar 28, 2024
View reviewed changes
Copy link
Contributor Author

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

The thing I was going for in the PR (which was hand-crafted instead of tool-assisted) was uniformity, and in this case following the convention of roughly half this file.

I'd rather have one way or the other, not some mishmash.

pkgs/development/lisp-modules-new-obsolete/ql.nix
Comment on lines 127 to 128
cl-readline = pkg: {
nativeLibs = [ pkgs.readline ];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More pkgs.

pkgs/development/lisp-modules-new-obsolete/ql.nix
Comment on lines +223 to +230
builtQlpkgs = lib.mapAttrs (n: v: build v) qlpkgs;

build = pkg:
let
builtPkg = build-asdf-system pkg;
withExtras = pkg //
(optionalAttrs
(hasAttr pkg.pname extras)
(lib.optionalAttrs
(lib.hasAttr pkg.pname extras)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the added lib. to see how it feels (vs. the tool-assisted extraction present in other PRs.)

@fricklerhandwerk
Copy link
Contributor

I get that. I mean, you're doing a refactor, and I appreciate you're doing it conservatively, but in this case we could as well make an opinionated choice - the original motivation was increasing clarity and readability. And again, I think judicious use of with can help with that.

@philiptaron
Copy link
Contributor Author

philiptaron commented Mar 28, 2024 •
edited

I'm sorry, I find a with scope going over 90% of the lines of the file, mixed in with use of pkgs. within that scope to not be code I wish to produce. I can't apply that opinionated feedback in this PR, as it's not an opinion I share.

The only place I find with to make sense is where the scope is minimal: a list, a small set like meta, the like. To me, that's a judicious use. If it scrolls off the screen while reading the file, or won't be present in the diff because it's far away, it's effectively a top-level with... and I would avoid it.

You can see evidence of that confusion when code authors introduce smaller with scopes that were already covered by some larger one. with lib at the top scope, followed by with lib in the meta section. Use of pkgs. in a list when there's a top-level with pkgs;.

The other possible use of with pkgs; that I could have transformed in this PR is putting with pkgs; in front of every nativeBuildInputs or buildInputs. I chose not to do that since virtually every one of these lists contain just one member, and adding pkgs. is shorter. It's kind of nit-picky, but shorter is better in this case I feel!

@fricklerhandwerk
Copy link
Contributor

Yeah, fair.

@philiptaron philiptaron requested a review from hraban April 3, 2024 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fricklerhandwerk fricklerhandwerk fricklerhandwerk approved these changes

@hraban hraban Awaiting requested review from hraban

Assignees
No one assigned
Labels
2.status: merge conflict 10.rebuild-darwin: 0 10.rebuild-linux: 0 12.approvals: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@philiptaron @fricklerhandwerk @wegank