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

feat: Add chezmoi:template:format-indent template directive #4163

Merged
merged 2 commits into from
Dec 29, 2024

Conversation

twpayne
Copy link
Owner

@twpayne twpayne commented Dec 27, 2024

Fixes #3873.
Implements @bradenhilton's suggestion in #3875 (comment).

This still needs some work, so this is draft PR.

@twpayne twpayne changed the title feat: Add chezmoi:template:indent template directive feat: Add chezmoi:template:format-indent template directive Dec 28, 2024
@twpayne twpayne marked this pull request as ready for review December 28, 2024 16:07
Copy link
Collaborator

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

The changes look good. I haven't tested them myself, but I think it's worth asking whether we want to (1) have two directives instead of one, and (2) be strict on string values to prevent footguns.


chezmoi:template:format-indent=$VALUE

`$VALUE` can be an integer number of spaces or a string.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this might not be better as two directives, format-indent-width and format-indent. They would be mutually exclusive. format-indent-width would be the integer number of spaces to use per level; format-indent would a a string, but would have to match ^[ \t]+$ to be valid. That is, format-indent="" would not be valid, nor would format-indent="..".

(One could argue that it should be (^ +$|^\t+$) to prevent mixing tabs and spaces.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, done. I've not been super-strict with the parsing, but any errors should be quickly visible to the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to be strict about only allowing tabs or spaces, would something similar to how it's done in EditorConfig be better? i.e.

chezmoi:template:indent-style:spaces
chezmoi:template:indent-style:tabs
chezmoi:template:indent-size:4

# and possibly also
chezmoi:template:tab-width:4

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think we need to be super-strict on this, and that it should be sufficient to need only one directive to specify the indent. So, I'll leave this as-is.

Copy link
Collaborator

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

Looks good.

@twpayne twpayne merged commit de8efad into master Dec 29, 2024
30 checks passed
@twpayne twpayne deleted the template-indent branch December 29, 2024 11:58
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.

3 participants