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

top-level: missing parentheses #21324

Merged
merged 1 commit into from
Dec 22, 2016
Merged

top-level: missing parentheses #21324

merged 1 commit into from
Dec 22, 2016

Conversation

elitak
Copy link
Contributor

@elitak elitak commented Dec 21, 2016

Motivation for this change

Without this change, when config.platform is set, the evaluator tries to apply it as a function to system.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@elitak, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ericson2314 and @nbp to be potential reviewers.

@Ericson2314
Copy link
Member

Sheesh sorry about that. I do wish or syntax wasn't so picky though.

@edolstra what would you think of e.g. ?? or some other non-alphanumeric infix-looking thing as a replacement for or to avoid conflicts with valid identifiers?

@grahamc
Copy link
Member

grahamc commented Dec 22, 2016

@Ericson2314 should we merge this and then debate the language?

@Ericson2314 Ericson2314 merged commit b6f8b0d into NixOS:master Dec 22, 2016
@Ericson2314
Copy link
Member

@grahamc. Yes. I was going to say I miss my "or" being lined up. But without the lack of parents to make that elegant I shouldn't be so picky.

@elitak elitak deleted the parens branch December 22, 2016 03:19
@vcunat
Copy link
Member

vcunat commented Dec 22, 2016

Or is a very high-priority operator, but similar breaking changes are rather "hard to do" nowadays.

@Ericson2314
Copy link
Member

@vcunat yeah then using a new previously keyword will solve the awkward overlap with indents and precedence problem in a backwards-compatable way.

@vcunat
Copy link
Member

vcunat commented Dec 22, 2016

The indents are a non-issue here, if I understand what you meant. or is just associative as one would expect. It should work perfectly well to have

args.platform
or config.platform
or ((import ./platforms.nix).selectPlatformBySystem system)

The problem addressed by this PR was that or has a higher priority than a function call, so you do need the (outer) parentheses on the last line above.

@Ericson2314
Copy link
Member

Sorry meant idents in that or is also a valid identifier.

@vcunat
Copy link
Member

vcunat commented Dec 22, 2016

I can't see how adding an operator would affect the fact that or is a special identifier, but I don't find that very significant, as the name doesn't seem that much useful to me.

@Ericson2314
Copy link
Member

My hope is special or would be deprecated and then removed, leaving it just another normal identifier.

@Ericson2314
Copy link
Member

There is a lib.trivial.or that I assume is the reason "or" is still a valid identifier. We could rename that function, but I soon as well keep it as a) I'm not a fan of alphabetical infix operators b) there is a matching lib.trivial.and.

@vcunat
Copy link
Member

vcunat commented Dec 22, 2016

I knew those, but they seem neither particularly useful nor much used. (Contrary to the "defaulting or" operator.)

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

Successfully merging this pull request may close these issues.

None yet

5 participants