Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

Support "max_line_length=off" #7

Closed
chipironcin opened this issue May 17, 2019 · 18 comments · Fixed by #10
Closed

Support "max_line_length=off" #7

chipironcin opened this issue May 17, 2019 · 18 comments · Fixed by #10

Comments

@chipironcin
Copy link

chipironcin commented May 17, 2019

Editorconfig supports off as a valid value for max_line_length (see https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#user-content-max_line_length)

Current code only sets the Prettier property printWidth if max_line_length !== "off":

  if (editorConfig.max_line_length && editorConfig.max_line_length !== "off") {
    result.printWidth = editorConfig.max_line_length;
  }

Ideally max_line_length = "off" should be supported, maybe with a very high value. Also other non-numeric values for it should be discarded.

I can raise a PR if this issue makes sense to the author and contributors. Please leave your feedback ;)

@josephfrazier
Copy link
Owner

That makes sense to me, @chipironcin , I'd love to see a PR for this!

@willmendesneto
Copy link

@josephfrazier This change is already added to the package's codebase. Would you mind closing this issue, please?

@josephfrazier
Copy link
Owner

@willmendesneto can you clarify how it has already been added? There haven't been any commits to this repository since this issue was opened.

@celluj34
Copy link

@josephfrazier

I believe they are referencing https://github.com/josephfrazier/editorconfig-to-prettier/pull/3/files which only makes it ignored.

@josephfrazier
Copy link
Owner

josephfrazier commented Mar 13, 2023

I'm trying to remember the context from 3+ years ago, but I believe the original request was to set result.printWidth to a very high value, or possibly preferably a special value that prettier interprets to mean Infinity, but it's not clear whether either approach would have the desired effect, according to https://prettier.io/docs/en/options.html#print-width:

For readability we recommend against using more than 80 characters:

In code styleguides, maximum line length rules are often set to 100 or 120. However, when humans write code, they don’t strive to reach the maximum number of columns on every line. Developers often use whitespace to break up long lines for readability. In practice, the average line length often ends up well below the maximum.

Prettier’s printWidth option does not work the same way. It is not the hard upper allowed line length limit. It is a way to say to Prettier roughly how long you’d like lines to be. Prettier will make both shorter and longer lines, but generally strive to meet the specified printWidth.

Remember, computers are dumb. You need to explicitly tell them what to do, while humans can make their own (implicit) judgements, for example on when to break a line.

In other words, don’t try to use printWidth as if it was ESLint’s max-len – they’re not the same. max-len just says what the maximum allowed line length is, but not what the generally preferred length is – which is what printWidth specifies.

@celluj34
Copy link

Ha, yes it was a long time ago. Sorry to necro such an old thread, but I've been running into this lately.

Does Prettier even support max_line_length=off? I couldn't find anything definitive either way.

@josephfrazier
Copy link
Owner

josephfrazier commented Mar 14, 2023

https://www.youtube.com/watch?v=Zmp_--Oow5o (whoops, mispasted) Ha, yes it was a long time ago. Sorry to necro such an old thread, but I've been running into this lately.

It's all good, happy to help if I can!

Does Prettier even support max_line_length=off? I couldn't find anything definitive either way.

No, based on what I quoted in my previous message, Prettier cannot support max_line_length=off, given that it prefers to make lines the given printWidth. However, if you can elaborate on the difficulties you're having, maybe we can find another way to solve your problem. Have you tried raising the max_line_length to the highest value you're willing to tolerate on your screen?

@celluj34
Copy link

However, if you can elaborate on the difficulties you're having, maybe we can find another way to solve your problem.

Sure, just that I have a 'global' ([*]) line limit at '120', but [xml] at 'off'. But what I'm getting is 120 for XML files.

Have you tried raising the max_line_length to the highest value you're willing to tolerate on your screen?

That's what I'm doing now, yeah, 9999, but that feels hacky.

IMO prettier should support off, which isn't the responsibility of this repo (but then you can forward off to it), or this tool could provide some large value for me.

@josephfrazier
Copy link
Owner

Sure, just that I have a 'global' ([*]) line limit at '120', but [xml] at 'off'. But what I'm getting is 120 for XML files.

Got it, thanks for the details!

That's what I'm doing now, yeah, 9999, but that feels hacky.

IMO prettier should support off, which isn't the responsibility of this repo (but then you can forward off to it), or this tool could provide some large value for me.

Hmm yeah, I see how it feels hacky. If Prettier did start supporting off as a value, I'd be happy to have this library pass it along, but I could be convinced to use a high value like 9999 instead (or probably Number.MAX_SAFE_INTEGER instead, so that you and others wouldn't have to put the hackiness into your .editorconfig files.

Let me think a little more about whether I want to open an issue in https://github.com/prettier/prettier/ to discuss this idea, or whether I want to go ahead and just do it, then open a PR to bump Prettier's dependency on this library. I'll get back to you later this week

@josephfrazier
Copy link
Owner

but I could be convinced to use a high value like 9999 instead

Well... it sounds like this could result in Prettier collapsing a bunch of lines into one really long line, based on the docs quoted above:

Prettier will make both shorter and longer lines, but generally strive to meet the specified printWidth.

So, I'm not sure if it's a good idea after all, might need to open that issue on the Prettier repo to discuss the implications.

@celluj34
Copy link

That is a good point - although I haven't seen any problems with the XML files I'm working with.

I would assume line length is context-dependent, as in it wouldn't try to put several JS lines delimited by a semicolon in a single line, but I don't know their internals.

Please let me know if you need anything from me!

@josephfrazier
Copy link
Owner

I just opened an issue in the Prettier repo and tagged you in it, we'll see what the other maintainers say!

@celluj34
Copy link

Nice! Thank you!

@josephfrazier
Copy link
Owner

josephfrazier commented Mar 16, 2023

@celluj34, I've published v1.0.0 with this feature, and now the ball is in Prettier's court to upgrade to it (I may continue helping there!). See here for more: prettier/prettier#14514 (comment)

I think it's the right thing to do from perspective of editorconfig-to-prettier. We can decide accept it or not when upgrading it. We can choose to ignore after all https://github.com/prettier/prettier/blob/d797f0cff203e4894295e9a021e55f8feefc89ab/src/config/resolve-editorconfig.js#L14-L15😄

Perfect, I've published v1.0.0 with the change that converts max_line_length=off to printWidth: Number.POSITIVE_INFINITY, and started on a PR to upgrade Prettier to that version: #14516

@celluj34
Copy link

Yep, I've been watching! Thanks for getting it all done. Once that part's complete I just need VSCode to upgrade the version of prettier it's using...

@josephfrazier
Copy link
Owner

Great, hopefully we're close to having this fixed for you 👍🏻

@josephfrazier
Copy link
Owner

josephfrazier commented Mar 18, 2023

@celluj34, prettier/prettier#14516 was merged, so I think the next release of Prettier should include this fix, unless it gets reverted for some reason.

@josephfrazier
Copy link
Owner

Prettier v2.8.5 has been released with the fix! https://github.com/prettier/prettier/blob/ddf3b43c33e2e98f6413b5232ad623876d96738e/CHANGELOG.md#support-max_line_lengthoff-when-parsing-editorconfig-14516-by-josephfrazier

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants