-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
RFC 101/166 style #118
Merged
Merged
RFC 101/166 style #118
Changes from 123 commits
Commits
Show all changes
128 commits
Select commit
Hold shift + click to select a range
d2cc629
Apply hlint suggestions
Lucus16 d83a00c
Generalize parsing utilities
Lucus16 c8ebd02
Attach leading trivia to next token
Lucus16 0c9d9a4
Add test for if-with-comments
Lucus16 f702d73
Fix argument order in Ann
Lucus16 5ae8e62
Allow standalone comments in lists and sets
Lucus16 4a05494
Add direnv
piegamesde 13b7904
Update CLI flags description
piegamesde f938cfc
Add simple test runner
piegamesde 9ce4ce3
Rework function declarations
piegamesde e05d6a2
Force-expand lists with more than one item
piegamesde 305ed77
Expand let statements more
piegamesde 73e7ec4
Rework if statements
piegamesde cd0acd3
Don't indent `in` body anymore
piegamesde 8315ca3
Rework `inherit` statements
piegamesde 28a54b3
fixup! Rework if statements
piegamesde 7fe6a99
Add some code documentation
piegamesde e9cdac6
Rework bindings
piegamesde 2e2797a
Tests: replace diff/idioms_pkgs_3
piegamesde dafec15
fixup! Rework bindings
piegamesde de7bd6d
Expand singleton lists with a multiline item
piegamesde d1ba3cd
Test: add idioms_nixos_2
piegamesde e6bed2a
Don't absort `in` body anymore
piegamesde a226b6d
Improve group handling for bindings and inherit
piegamesde 0528d78
Special case selection operator
piegamesde 8a6cb8c
Rework operators
piegamesde 487d313
Rework parentheses
piegamesde 6ce18ad
Rework function calls
piegamesde 3802eaa
Add direnv
piegamesde 7c18e90
Merge branch 'leading-comments' into rfc101-style
piegamesde 10745f3
Fix function commas
piegamesde 20d35b3
Move comments around a bit
piegamesde 2a9ec6b
Tweak operations some more
piegamesde cb309e4
wip
piegamesde 6f4791d
pretty parentheses
piegamesde 8d07dc9
stupid silly bug
piegamesde b1b9313
Absorb parenthesized abstractions with multiple arguments
piegamesde 0b26acb
Tweak operations some more
piegamesde 3e16d20
Don't force-expand (simple) if statements anymore
piegamesde db2c938
Special case binary operators
piegamesde 66ac4b4
Improve error message on verify
piegamesde 5a8eb61
Improve layouting algorithm
piegamesde eeb2534
Factor out function application code
piegamesde 792c405
Infinisil style function application
piegamesde b7daac9
Improve comment handling
piegamesde b86d8ea
Unindent semicolons again
piegamesde 9a6cc7f
Don't absorb lambda body
piegamesde 3da135d
Absorb parenthesized function calls again
piegamesde 66b6712
Binder with with: absorb less
piegamesde 5bb0639
Rework sets and lists
piegamesde f7cf76f
Force-spread inherit with more than three items
piegamesde a7dc8bb
Binder: Always absorb strings and paths
piegamesde 4e1ff44
Tests: add lib/systems/parse.nix
piegamesde c926692
Better trailing comment parsing
piegamesde c2702df
Function application: fix edge case
piegamesde 538663c
Function application: don't always absorb last argument
piegamesde 325305e
Binders: force-expand nested attribute sets
piegamesde a95bd3a
Abstraction: don't absorb body when there are more than two parameters
piegamesde 32609c8
Ignore comments in line length calculation
piegamesde c568bfa
Ignore indentation in line length calculation
piegamesde 1e42aa2
Merge remote-tracking branch 'upstream/master' into rfc101-style
piegamesde e6693ca
Absorb abstraction in binder
piegamesde 3b26c7d
Improve mapFirstToken code style
piegamesde 4c0007e
Tests: Add check-meta.nix
piegamesde ef310bb
Improved helper functions
piegamesde a54a01a
Improve priority group handling
piegamesde c4bfa2c
Rework `//` operator
piegamesde 81d3cf8
Copy special cases over to `++` operator
piegamesde cc5b426
Introduce support for optional trailing commas
piegamesde 4468e9b
Expand attrset function parameters less
piegamesde 9ac66a9
Rework renderer again
piegamesde a93dcf7
Binders: be more selective about semicolon placement
piegamesde 43dd01d
Fix comment handling
piegamesde 549541f
Expand singleton lists again
piegamesde 7649a1b
Put semicolons on same line again
piegamesde 8645a22
Update the default line length to 100
piegamesde d2d4a54
Tests: add make-derivation.nix
piegamesde 8a5109b
Types: introduce mapLastToken
piegamesde 55aae75
Function declarations: fixup commas and comments more
piegamesde 91acfa0
Tests: add interpolation test
piegamesde 064bf4d
String interpolations: Ignore line length limits
piegamesde 9d7317f
String interpolations: compact function applications
piegamesde 46d12b2
Function application: Compact simple functions
piegamesde 62fe12f
String interpolations: Fix indentation
piegamesde 951261e
Strings: Fix single-line strings with double single quotes
piegamesde 136edf4
Inherit: Indent trailing semicolon again
piegamesde 206653b
List, Attrset: Remove surrounding spaces
piegamesde 27ce996
Fix idempotency, enable --verify in tests
piegamesde 21ef16e
Move some more comments up
piegamesde fda8afa
Function application: Fix indentation with multiline function arguments
piegamesde 0a8c246
Revert "List, Attrset: Remove surrounding spaces"
piegamesde 35da232
Strings: Fix multi-line strings that end with a single quote
piegamesde a273e5a
Lists: Fix absorbtion rules
piegamesde 070063e
Assert: always force-expand
piegamesde 53c7361
Rework: with, paramAttr, bindings, parentheses
piegamesde 067c281
Force-expand attrsets in attrset functions
piegamesde d6930fd
Refactoring
piegamesde 08fe739
Fix false positive in --verify checks
piegamesde bb45962
Treat some parenthesized expressions as "simple"
piegamesde a75658d
Strings: Don't normalize anymore
piegamesde 543b65f
WIP: Automatically minimize verification failures
piegamesde 5ab9430
Relax dependency version bounds
piegamesde 887a40d
Update Nix dependencies
piegamesde 6cfbc2c
Fix TypeOperators warning
infinisil bbe0ba5
Improve multiline asserts
piegamesde eb732b1
Rework indentation handling
piegamesde 62b0c39
Small cleanup
piegamesde d1067e4
Predoc: Support multiple priority groups
piegamesde 7b1fcf5
Function application: Expand non-last arguments when rest fits onto a…
piegamesde a273a47
Add some tests
piegamesde 9ec2ac0
If: Move trailing comments around
piegamesde aa85d38
Predoc: Prevent trailing whitespace on lines
piegamesde d531cef
With: Don't absorb when there is a trailing comment
piegamesde 8f4bf7e
Abstraction: Don't absorb when there is a trailing comment
piegamesde 3b95476
Binding: Small refactoring & add test
piegamesde bcbf8c6
Binding: Don't absorb // and ++ when there is a trailing comment
piegamesde d6902b0
Fix indentation
piegamesde 6413a12
List, Attrset: Preserve empty line when there are no items
piegamesde 6bd07b5
Fix parenthesized function application
piegamesde 22fa00e
Parentheses: Move trailing comments up and in
piegamesde f7d9cfe
Application: add some special cases
piegamesde 25792d4
Comments: use RFC style
piegamesde 2b5ee82
String interpolation: Indentation fixes
piegamesde 7d99e0b
Merge branch 'master' into rfc101-style
infinisil 4eb99c1
Use /usr/bin/env shebang in tests
infinisil 08b34c0
Comply with REUSE
infinisil f43ec86
Remove accidentally committed file
infinisil 8d13b59
Parser: Fix operator parsing
piegamesde File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
use_nix | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,4 @@ | |
/dist-newstyle | ||
/.ghc.environment.* | ||
/result | ||
/.direnv |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this added? Should it be up to the user whether or not they would want to use this specific default Nix shell? Committing this to the repo means anyone with direnv installed is going to get that big red consent message after cd-ing into the project for the first time. This also makes this hard to extend if users have other environment needs & now have to deal with potentially accidentally committing those changes as this personal environment file is now checked into the repository.
I would prefer this be moved to
.envrc.example
but given it is a single line & those with direnv already know how to use it, a line could be added to the README or flake that says enabling isecho "use flake" > .envrc && direnv allow
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
direnv deny
? Also works withdirenv disallow
,direnv block
, anddirenv revoke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't understand your point. Committing the
.envrc
file is the point of direnv, to make the developer experience smoother. I love it when projects do that. The shell is additive and not pure, so it will take the outside environment into account.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t the experience a user wants. They want to enable direnv, but only after they have a) had time to inspect the .envrc file & b) made modifications for themselves. Current state throws up ANSI red prompt just for cd-ing into the directory. Allow should happen after everything is ready.
Scenario: You and I have different setup needs. I need different environment variables & scripts specific to me. The only way to get them for my personal setup is to edit the .envrc. Now that I have edited the .envrc,
git status
is saying I have made modification even tho they should be specific to me. I might also accidentally commit those changes as a result since it’s a tracked file. I believe the point is to set up your environment for you, not for everyone as not everyone has the same environment. The point of an.envrc.example
is the place for a suggested setup for the average user.cp .envrc.example .envrc
is one tiny step. Many projects have.env
in their.gitignore
because they don’t want to a) check in sensitive env info b) prescribe everyone have the same setup. This is as weird as committing your personal text editor settings… they are your environment settings, not everyone else’s.I hate it. If I just need to edit a README, I don’t want a direnv prompt, & certainly don’t want to copy down another version of Nixpkgs with tooling I won’t be needing for my task, and definitely not automatically running arbritrary executables I don’t yet understand because I just cloned the repo. I do like it when they offer an example I can copy, or if there is nothing interesting but
use flake
oruse nix
, just stick it in the docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user, I expect that when I install a tool (direnv) and a repo supports it (nixfmt), then the tool should be used. Don't want direnv? Don't install it, then.
Great, direnv has support for that:
$XDG_CONFIG_HOME/direnv/direnvrc
and$XDG_CONFIG_HOME/direnv/lib/*.sh
. We could also do this but that has security implications.To be honest, I have never heard from any developer who has direnv installed that they don't want to use it (why did they install it then?). To me it's as simple as:
direnv deny
it once. It's quick and easyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please don't block this PR on that, I think everyone can handle whatever outcome we end up with 🙈 Thank you for your work!! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dasJ Late to the party, but I just wanted to say I strongly discourage adding
.envrc
to a git repo (or any other VCS), and it's not about havingdirenv
installed and not wanting to use it btw :) Here are some counter arguments:.envrc
file..envrc
, e.g. I use fish shell and it requires some tweaks on certain repos, other folks use zsh or bash, etc.Having this file committed to the repo removes this flexibility, but it's also easier to argue in its favor for an open source public repo. For private repos there's even more counter arguments, for instance, we have this at work:
What I suggest instead
Let the users decide what they want available in their environments while still leveraging
direnv
, be a good citizen and add.envrc
to your global.gitignore
or add it to the project's.gitignore
to avoid anyone adding it back.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a good project shell would assume no such things
Regarding credentials: these should not be set in .envrc, but project shell should provide tools (scripts, commands) to manage these
These can be entered ad-hoc with nix shell\develop
A command (provided by default dev shell) should be provided to switch between envs
.envrc IMHO should never be edited by devs, it should simply setup all tools needed to work on given project. If customisation of env vars is needed this could be loaded from optional, gitignored file(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project assumes I need a specific HLS version
Agree to disagree. The methods vary across different projects when working on a big organization. A lot of flows are very complex and unfortunately can't be automated, e.g. sign in via SSO behind a VPN, approving the sign in attempt via Okta / Duo, etc. It barely works on Linux, let alone trying to automate these things.
It's much easier on open source public projects, I'll give you that, but private organizations are a completely different story.
It doesn't make it easier to pin specific pkgs, add overlays, etc.
This isn't bad actually, might work well in some cases, will give it a try :) Certainly not in cases where switching envs requires going through the complex authorization flows, though.
How would you do this if you don't control the content of
.envrc
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've had this topic at least thrice already. Not sure having this discussion yet another time is a productive use of anybody's time