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

Allow any indent size from 1 to 16 #7429

Merged
merged 3 commits into from
Jun 23, 2023
Merged

Conversation

msdrigg
Copy link
Contributor

@msdrigg msdrigg commented Jun 22, 2023

Previously, helix only allowed indent sizes up to 8. This commit changes to allow any indent sizes that fit within a u8.

This pr is motivated by a hackernews user with a deranged normal desire to have a 12 space indent.

See request here: https://news.ycombinator.com/item?id=36429221

Code motivated by suggestions here: https://news.ycombinator.com/item?id=36432996

I used a single line of 256 spaces because it's super simple and easy to validate rather than using an additional dependency like const_format or writing a macro myself.

Previously, helix only allowed indent sizes from 1-8, but this isn't anything fundamental. This commit changes to allow any indent sizes that fit within a u8.

This commit is motivated by a hackernews user who has a deranged desire to have a 12 space indent.

See request here: https://news.ycombinator.com/item?id=36429221

Code motivated by suggestions here: https://news.ycombinator.com/item?id=36432996
@kirawi
Copy link
Member

kirawi commented Jun 22, 2023

#7178 the limit is 16, so the static string should be limited to that.

@pascalkuthe
Copy link
Member

That's tab width and not indent width. That said I still think that a value of 16 is large enough.

Unfortunately, my first PR was too deranged for the maintainers of
helix. :(

I have restricted the indents to be only 1-16.
Now clamping both in `IndentStyle`'s `to_str` and
`from_str` methods.

Also added a constant to be clearer where the random
17's and 16's are throughout the file.
@msdrigg
Copy link
Contributor Author

msdrigg commented Jun 22, 2023

Okay, all suggestions fixed and clippy should be happy. I also added a constant so it's clear why the literals were chosen, but I can put it back to literals if it's preferred.

Also, I didn't see a good way to test the failing case where indent > 16 specifically because behavior could vary between debug and release builds (debug will panic for indent > 16 while release will pass with indent = 16)

@the-mikedavis the-mikedavis changed the title Allow any indent size from 1 to 256 Allow any indent size from 1 to 16 Jun 23, 2023
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 23, 2023
@archseer archseer merged commit 93ac706 into helix-editor:master Jun 23, 2023
6 checks passed
@msdrigg msdrigg deleted the huge-indents branch June 23, 2023 19:25
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants