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.

Sign up for GitHub

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

Jump to bottom

Add check for a non-zero value for tab width #7178

Merged
merged 3 commits into from
Jun 7, 2023
Merged

Add check for a non-zero value for tab width #7178

merged 3 commits into from
Jun 7, 2023

Conversation

bo1led-owl
Copy link
Contributor

@bo1led-owl bo1led-owl commented May 30, 2023 •
edited
Loading

Closes #7094. I think it is weird to set tab width to zero, so I added a custom deserialization function (a small wrapper that checks if the tab width value is zero).

@bo1led-owl bo1led-owl changed the title Add check for a non-zero value for tab width (#7094) Add check for a non-zero value for tab width May 30, 2023
@kirawi
Copy link
Member

kirawi commented May 30, 2023

I think you can use NonZeroUsize instead.

@bo1led-owl
Copy link
Contributor Author

Thank you for the advice, I didn't know about this feature. I'll take some time to rewrite it

@bo1led-owl
Copy link
Contributor Author

bo1led-owl commented May 30, 2023 •
edited
Loading

I changed it, now it's more clear and also fixes the original issue

@kirawi kirawi added C-bug Category: This is a bug A-helix-term Area: Helix term improvements labels May 31, 2023
@gabydd gabydd mentioned this pull request May 31, 2023
Closed
pascalkuthe
pascalkuthe reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

While you are at it could you limit the value to something reasonable like 16 at most?I am pretty sure the code doesn't correctly account for tab-width larger than 8 and I can't think of any usecase

@bo1led-owl
Copy link
Contributor Author

bo1led-owl commented Jun 1, 2023 •
edited
Loading

While you are at it could you limit the value to something reasonable like 16 at most?I am pretty sure the code doesn't correctly account for tab-width larger than 8 and I can't think of any usecase

Yes, I think that it's a reasonable change.
Also, am I doing the right thing by making this a startup error? I was thinking about it but couldn't really come to a conclusion. I think it's better to make it an error like that, because it breaks the rendering.

pascalkuthe
pascalkuthe approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment •
edited by the-mikedavis
Loading

Choose a reason for hiding this comment

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

Yeah deserialization errors is how we usually handle invalid config. LGTM 👍

@pascalkuthe pascalkuthe added the S-waiting-on-review Status: Awaiting review from a maintainer. label Jun 1, 2023
@archseer archseer merged commit 77e9a22 into helix-editor:master Jun 7, 2023
Triton171 pushed a commit to Triton171/helix that referenced this pull request Jun 18, 2023
@bo1led-owl @Triton171
Add check for a non-zero value for tab width (helix-editor#7178)
19a5ee2
@kirawi kirawi mentioned this pull request Jun 22, 2023
Merged
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@bo1led-owl @wes-adams
Add check for a non-zero value for tab width (helix-editor#7178)
3a7ea39
@kirawi kirawi mentioned this pull request Nov 8, 2023
Open
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
@bo1led-owl @mtoohey31
Add check for a non-zero value for tab width (helix-editor#7178)
3a0c8bd
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
@bo1led-owl
Add check for a non-zero value for tab width (helix-editor#7178)
485289b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirawi kirawi kirawi approved these changes

@pascalkuthe pascalkuthe pascalkuthe approved these changes

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add not zero check on tab-width.
4 participants
@bo1led-owl @kirawi @pascalkuthe @archseer