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

define a variable for the amount of indented spaces #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

toastal
Copy link
Contributor

@toastal toastal commented Jun 26, 2024

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 2 integers thru the code.

1

Footnotes

  1. 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.

Copy link

github-actions bot commented Jun 26, 2024

Nixpkgs diff

@dasJ
Copy link
Member

dasJ commented Jun 26, 2024

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

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I think that would be a great follow-up PR. Let's favor smaller PRs and merging often 😄

LGTM!

Copy link
Member

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

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

Indentation handling is one of the most brittle parts of Nixfmt currently. Any code that touches this should have a strong testing story to avoid regressions for non-default indentation widths

src/Nixfmt/Predoc.hs Outdated Show resolved Hide resolved
src/Nixfmt/Pretty.hs Outdated Show resolved Hide resolved
@@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/Nixfmt/Pretty.hs Outdated Show resolved Hide resolved
src/Nixfmt/Predoc.hs Show resolved Hide resolved
@toastal
Copy link
Contributor Author

toastal commented Aug 13, 2024

@infinisil why is this a draft?

@infinisil
Copy link
Member

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

@toastal
Copy link
Contributor Author

toastal commented Aug 13, 2024 via email

@piegamesde
Copy link
Member

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)

@toastal
Copy link
Contributor Author

toastal commented Aug 13, 2024 via email

@piegamesde
Copy link
Member

piegamesde commented Aug 13, 2024

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 layout function.

@infinisil
Copy link
Member

What is unready? I pushed my changes 2 months ago & never got another review

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.

@toastal
Copy link
Contributor Author

toastal commented Aug 15, 2024

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 2s all over the place (hence needing suggested changes since it is unclear what is indentation 2s & what is other 2s). I think moving indentation to a variable is 100% a ‘meaningful’ step in the direction of configuration as well as others to understand which 2s are indentation, but configuration via CLI was never a goal of this patch.

define a variable for the amount of indented spaces

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.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

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?

src/Nixfmt/Predoc.hs Outdated Show resolved Hide resolved
src/Nixfmt/Pretty.hs Outdated Show resolved Hide resolved
@infinisil infinisil marked this pull request as ready for review August 26, 2024 20:48
@infinisil
Copy link
Member

Waiting for @toastal's confirmation before merging

@toastal
Copy link
Contributor Author

toastal commented Aug 27, 2024

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 2s are which) is the obvious first step before making the indentation level configurable. Maintainers are allowed to fixup attempts for contributors on their behalf--especially if the language isn’t their primary one.

@infinisil
Copy link
Member

Confirmation on?

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?

@infinisil
Copy link
Member

infinisil commented Aug 27, 2024

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
@Vortriz
Copy link

Vortriz commented Oct 23, 2024

Pinging, because there has been no update on this for the past 2 months.

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

Successfully merging this pull request may close these issues.

5 participants