define a variable for the amount of indented spaces #209
Conversation
|
Do you think you could also wire up a CLI option to fix #77? This way people could use whatever indentation they want without having to patch nixfmt |
src/Nixfmt/Predoc.hs
Outdated
| @@ -534,7 +539,7 @@ layoutGreedy tw doc = Text.concat $ evalState (go [Group RegularG doc] []) (0, s | |||
| -- [ # comment | |||
| -- 1 | |||
| -- ] | |||
| Text _ _ TrailingComment t | cc == 2 && fst (nextIndent xs) > lineNL -> putText' [" ", t] | |||
| Text _ _ TrailingComment t | cc == indentWidth && fst (nextIndent xs) > lineNL -> putText' [" ", t] | |||
There was a problem hiding this comment.
This line is a workaround for a very specific quirk. I don't remember all the details, but after reading its documentation comment above I am not entirely certain as to how to adapt this to other indentation lengths, or even if this is necessary for other indentations in the first place.
My gut feeling says that either the issue only happens at indentation width 2, or that the workaround only works for indentation width 2. In the latter case, changing the value might introduce idempotency bugs again.
|
@infinisil why is this a draft? |
|
In the last meeting we decided to draft non-ready PRs. Feel free to undraft when it's ready Perhaps you could also sync with @omkumar312 on this, who showed interest in this work in #77 |
|
What is unready? I pushed my changes 2 months ago & never got another review
--
toastal ไข่ดาว | https://toast.al
PGP: 7944 74b7 d236 dab9 c9ef e7f9 5cce 6f14 66d4 7c9e
|
|
I don't think that this change meaningfully advances us towards the goal of having configurable indentation. The fundamental concerns in my review were not addressed. (Though apologies for not giving feedback earlier) |
|
Setting the indentation level as a variable means an easy substituteInPlace in the patchPhase which was quick fix rather than figuring out argparsing in Haskell & the rest of its toolchain.
--
toastal ไข่ดาว | https://toast.al
PGP: 7944 74b7 d236 dab9 c9ef e7f9 5cce 6f14 66d4 7c9e
|
|
I'm sorry but this really needs a sound and thorough testing story. Especially since there are suspected open edge cases which can only be found by fuzzing against lots of code like Nixpkgs (and also probably some unknown unknowns in addition to the known unknowns). I recommend looking at aff0520, which not only introduces a CLI option but also does some refactoring which makes it significantly easier to pass values from the CLI through to the |
Oh didn't see that, sorry. @piegamesde I don't think this needs testing as is, because it doesn't provide any user interface for it, or guarantee that it's gonna keep working. In essence, this PR just does an internal refactoring. |
Which was pretty much how I was seeing it since I don’t want to dedicate the time to the CLI flags & test, even if I would like to see it. A refactor like this IMO clearly makes sense over seemingly-arbitrary
This is the entire scope I was going for since it makes patching a fork for the indentation level a one-line patch. Since I am already using these patches for my own fork, it seemed like a ‘nice thing to do’ to try to share patches upstream if they can be used or at least inspire a similar changeset that would make patching easier for myself—or eventually allow overriding via flag. If y’all’re saying this phase 1 of moving to a CLI flag isn’t valuable then please close this as I won’t being doing anything beyond the scope of the patchset title. |
There was a problem hiding this comment.
I do understand @piegamesde hesitance, but I think it's fine to merge this with the explicit non-guarantee of correctness or support for a different value. To treat this as purely an internal refactoring and effectively ignoring that you might patch over this.
But if you do patch over this and there's issues with it, I'll ask that you spend some effort to properly implement this feature, instead of disguising it as another internal refactoring, ok?
|
Waiting for @toastal's confirmation before merging |
|
Confirmation on? I would be happy if anything gets merged … even rewriting my changes. It just seems clear to me that identifying where these spaces are indeed configurable (meaning which |
This:
|
|
I'll look into the CI issue, not sure what's up with that. Edit: #241 fixes it |
makes it easier to override or, hopefully in the future, make configurable
infinisil
force-pushed
the
indent-space
branch
from
afab86d
to
aa453b4
Compare
Makes it easier to override or, hopefully in the future, make configurable (weird you can configure the line length, but not indentation width).
Personally I find 2-space-indented code very difficult to read. I am maintaining a fork on Codeberg to help myself in my own code bases with this patch (but increased indentation width) but this means anyone that wants to also change the indentation only needs to patch a single line instead of multiple spots. This could be fixed if Nix were to support tabs, but that isn’t the case. It also just seems better practice to define the indentation than
2integers thru the code.1
Footnotes
Please consider giving up MS GitHub or offering a non-proprietary, non-US-corporate-controlled mirror for this free software project. I wish to delete this Microsoft account in the future, but I need more projects like this to support alternative methods to send patches & contribute. ↩