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

talonfmt: raise maximum line length to 88 #990

Merged
merged 6 commits into from
Oct 15, 2022
Merged

talonfmt: raise maximum line length to 88 #990

merged 6 commits into from
Oct 15, 2022

Conversation

phillco
Copy link
Collaborator

@phillco phillco commented Oct 2, 2022

Opening for discussion.

I didn't notice the line length parameter until after #947 merged; we didn't discuss it much in that PR.

After I started using talonfmt in my own repositories, I've personally found a higher maximum length to be more ergonomic, as it means more commands can fit on a single line, which in my experience makes them easier to edit (although obviously having an autoformatter helps a lot). I've used 100 in axkit and in my own private repository and personally quite like the results.

I thought I would open this PR to kick off the discussion. If 100 is too high for people, perhaps we could find a middle ground? For context, Black uses 88, so if we set 88 we will be consistent across both languages.

cc @wenkokke

@wenkokke
Copy link
Collaborator

wenkokke commented Oct 3, 2022

@phillco You could probably set up pre-commit to format with 100 on checkout and with 80 on commit?

@phillco
Copy link
Collaborator Author

phillco commented Oct 3, 2022

That would probably create a lot of spurious local changes, which probably would be detrimental. I think it should probably be consistent.

My intent here is to propose that we simply set a higher value globally.

@rntz
Copy link
Collaborator

rntz commented Oct 9, 2022

I prefer shorter lines because I like to put things side-by-side on a laptop monitor. Even on a 15" monitor with a small-to-my-eyes font, 100 is slightly too long to put two windows side-by-side (with line numbers). And with a more normal-sized font, even 90 chars is too wide, with 88 chars just scraping in. So if we want to increase it, I vote for 88. I'm perfectly happy with 80, though.

Example:

image

@rntz
Copy link
Collaborator

rntz commented Oct 9, 2022

If black uses 88 by default and we're already using black, then 88 has the advantage of being consistent across Python & .talon files as well :).

@phillco
Copy link
Collaborator Author

phillco commented Oct 9, 2022

I'd settle for 88 :)

@phillco phillco changed the title talonfmt: set maximum line length to 100 talonfmt: set maximum line length to 88 Oct 9, 2022
@phillco phillco changed the title talonfmt: set maximum line length to 88 talonfmt: raise maximum line length to 88 Oct 9, 2022
@pokey
Copy link
Collaborator

pokey commented Oct 10, 2022

We should prob update editorconfig while we're here https://github.com/knausj85/knausj_talon/blob/4d39608dc9979017d7a0218b195feb54ff136ffa/.editorconfig#L10

@phillco
Copy link
Collaborator Author

phillco commented Oct 10, 2022

Done. I also removed the redundant indent_style = space, indent_size = 4 under .py because that was specified by the default * section if I understand right; let me know if it's still needed.

@phillco phillco requested a review from nriley October 10, 2022 20:33
.editorconfig Outdated
Comment on lines 17 to 20
max_line_length = 88

[*.talon]
max_line_length = 88
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should just set max_line_length to 88 globally, under [*]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we keep 80 for [*.{md,yaml,yml}], or just use 88 everywhere for simplicity?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say everywhere for simplicity.

@rntz rntz merged commit 4612817 into main Oct 15, 2022
@rntz rntz deleted the talonfmt-100 branch October 15, 2022 14:00
nickvisut pushed a commit to nickvisut/knausj_talon that referenced this pull request Nov 1, 2022
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Michael Arntzenius <daekharel@gmail.com>
Spiteless pushed a commit to Spiteless/talonhub_community that referenced this pull request Nov 9, 2022
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Michael Arntzenius <daekharel@gmail.com>
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.

None yet

4 participants