Atom-beautify: Remove `unformatted` defaults as per bug fix released in js-beautify 1.8.0.

Created on 27 Aug 2018 · 15Comments · Source: Glavin001/atom-beautify

Good news! In https://github.com/beautify-web/js-beautify/issues/1033 we finally succeeded in getting a long-standing js-beatify bug fixed. Specifically js-beautify was mis-using the unformatted setting to indicate whether elements were inline or not. Without this set, inline elements such as <mark> would be broken into multiple lines. But with it set, js-beautify would not format these elements at all (e.g. it would not remove line breaks between attributes).

This fix how now available in js-beautify 1.8.0: https://github.com/beautify-web/js-beautify/releases/tag/v1.8.0

The solution was to make a new inline setting that the formatter really considers to be inline—that is, they are formatted, just not broken. All the inline elements formerly in unformatted were moved to inline. The unformatted was left, just in case someone still wants to add an element that really is not formatted at all, but it is set to empty. See https://github.com/beautify-web/js-beautify/pull/1407 .

Unfortunately atom-beautify sets its own default values for unformatted, so they will need to be removed or they will override these changes (essentially un-fixing the fix for https://github.com/beautify-web/js-beautify/issues/1033 ).

I think the real solution here is to remove the atom-beautify explicit values altogether, as I recommended in https://github.com/Glavin001/atom-beautify/issues/1008#issuecomment-228435542 . js-beautify has its own defaults. They have been changed multiple times, and if atom-beautify duplicates those defaults explicitly, atom-beautify will have to be playing catch-up with manual modifications each time. I think atom-beautify should allow anything to be overridden, but has no need to specify explicitly what atom-beautify is providing as defaults anyway.

In any case, atom-beautify needs to remove the explicit unformatted values set in https://github.com/Glavin001/atom-beautify/issues/1008 whenever atom-beautify updates to js-beautify v1.8.0.

Thanks!

help wanted pr-merged question

Source

garretwilson

All 15 comments

Please follow the issue template provided. More specifically, update the original comment for this issue by adding a link to the required debug.md gist which includes debugging information that answers our most commonly asked questions. Thank you.

issue-check[bot] on 27 Aug 2018

Sufficient information is provided in the description. There is no need for a debug.md gist.

garretwilson on 27 Aug 2018

👍1

Sorry about that, the issue-complete bot is a little over excited sometimes 😝 . cc @szeck87 .

Unfortunately, removing default values for options has already been investigated and determined to not be feasible within Atom: https://github.com/Glavin001/atom-beautify/issues/894#issuecomment-270056127
I had to hunt for that comment, way back in Jan 3, 2017 😮. Atom's Settings View may have changed since, however, I do not have time to reinvestigate this.

The applicable code for the default options can be found at https://github.com/Glavin001/atom-beautify/blob/bb9428a4fa483fa6ffe958cdcb519b27e00cb770/src/options.json#L365-L433

However, I am not necessarily in favour of making a breaking change -- updating the defaults -- as it is recommended to users of Atom-Beautify to explicitly override with their own option values and note again the default options are only there to appease Atom's Settings View schema validation. Atom-Beautify is not trying to be the best and single source of truth for default options. Atom-Beautify is trying to allow users the freedom to change these options in a similar way with a single package.

The solution was to make a new inline setting that the formatter really considers to be inline—that is, they are formatted, just not broken. All the inline elements formerly in unformatted were moved to inline. The unformatted was left, just in case someone still wants to add an element that really is not formatted at all, but it is set to empty. See beautify-web/js-beautify#1407 .

I only have a few moments right now -- however, I wanted to reply ASAP -- so I have yet to review js-beautify's issue fully.

At first glance, it sounds like a new inline option should be added to Atom-Beautify? If this is the case, I would be happy to review a small Pull Request submitted from an open-source community member, such as yourself @garretwilson . I do want to support the latest and greatest options, such as inline for js-beautify, and welcome the community to submit Pull Requests for @szeck87 and I to review.

The default values are not something I want to maintain and only a necessary evil, as mentioned above and demonstrated in https://github.com/Glavin001/atom-beautify/issues/894#issuecomment-270056127 . Given we cannot simply remove the default values and also doing so would be an unexpected breaking change for some, I would not approve a Pull Request implementing such a change without further consideration.

I'll try to review js-beautify later. Please comment here if you have any questions or suggestions. Thank you!

Glavin001 on 1 Sep 2018

@garretwilson @Glavin001
I thought I'd take a swing at fixing this, but I don't think I'm doing it right.

bitwiseman on 1 Sep 2018

🎉1

At first glance, it sounds like a new inline option should be added to Atom-Beautify?

Yes, but (and this is important!) you much change the defaults of unformatted to [].

Let me make it simple: unformatted should always have been empty by default. <a>, for example, should not be "unformatted". We want to format an <a> tag. I want to format <a>. You want to format <a>. We all want to format <a>. If there are any people (rare, I'm sure) who do _not_ want to format <a>, they can manually set it in their own unformatted configuration, like you said.

Then why did we put <a> in unformatted before? Because there was an atrocious bug that would format <a> (and everything else) as if it were a block element!! It would put newlines before and after <a>, and other (perhaps all other) inline elements. So we put all the inline elements in unformatted just to work around this bug.

This bug has (finally!!!!!!) been fixed. The inline elements are now in inline. So you need to do two simple things:

Add an inline setting that defaults to all the inline elements (i.e. the values unformatted used to have).
Set the default to unformatted to [].

There is no reason to keep unformatted to anything but [] now. If you do _not_ change the default of formatted to [], then that means _everybody who wants to format <a>_ (which is probably 99.997 percent of your users) will be forced to put "unformatted": "" in their configuration file or the file won't format correctly.

Even worse, if users don't know to put "unformatted": "" in their configuration file, they will think their files are formatting correctly until that rare instance comes along and their <a> has newlines in the middle of it, and atom-beautify won't format it, and they won't know why it's broken.

Please, _please_ set the default of unformatted to [] and not force all your users to set it manually.

garretwilson on 2 Sep 2018

Let me give an example, assuming that line-wrapping is turned off:

<p>This is a <a


    href="http://example.com/">link</a> to a site.</p>

Does that need formatting? Yes, of course. It should look like this:

<p>This is a <a href="http://example.com/">link</a> to a site.</p>

Can you find one single person who says that <a> should _not_ be formatted? I seriously doubt you can. (And if that one person exists, they can set "unformatted": ["a"].)

The reason we had <a> in the unformatted list before, is that there was a horrible bug that would format things something like this:

<p>This is a
  <a href="http://example.com/">link</a>
  to a site.
</p>

And so to work around the bug we chose the lesser of two evils and decided to tell js-beautify _not to format <a> at all_! (And we did it with <cite>, and <abbr>, and <code>, and <mark>, etc.) But that's not really what we wanted. That's not what anyone wants!

That bug has been fixed. We can now say, "Format <a> by default, but don't add newlines before and after it." If you only add the new inline setting, then you're only doing the second part: "Don't add newlines before or after <a>." But you'll still be leaving in a setting that says, "Don't format <a> at all!!" What's the point in that?

garretwilson on 2 Sep 2018

In order to show that I don't merely want to complain, but that I _do_ want to contribute, I've posted a USD$50 bounty for this ticket. I think @bitwiseman has already submitted a pull request; that's fine—he certainly deserves it for going out of his way to update another library that depends on his.

Here is the bounty for whoever completes this task:

https://www.bountysource.com/issues/62674449-remove-unformatted-defaults-as-per-bug-fix-released-in-js-beautify-1-8-0

I'm so anxious to get this fixed and I'll be overjoyed when it's in. Thank you in advance.

garretwilson on 2 Sep 2018

@garretwilson
As usual you make a thorough case for your desired change.

FYI, you (and any other user) can get the behavior you want by setting unformatted to empty right now. Since atom-beautify doesn't currently _set_ inline, it will use the default from js-beautify. That should reduce the personal urgency of this change. :).

bitwiseman on 2 Sep 2018

As if I didn't have enough to complain about: I just now uninstalled atom-beautify and tried to install it again, to pull down the latest js-beautify. I can't even install atom-beautify back in Atom!! I get the following:

npm WARN deprecated [email protected]: please upgrade to graceful-fs 4 for compatibility with current and future versions of Node.js
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated @types/[email protected]: This is a stub types definition for commander (https://github.com/tj/commander.js). commander provides its own type definitions, so you don't need @types/commander installed!
npm WARN deprecated [email protected]: Deprecated in favour of eslint-config-wikimedia. -- https://phabricator.wikimedia.org/T118941
npm WARN deprecated [email protected]: Package no longer supported. Contact [email protected] for more info.
npm ERR! Windows_NT 10.0.17134
npm ERR! argv "C:\Users\user\AppData\Local\atom\app-1.30.0\resources\app\apm\bin\node.exe" "C:\Users\user\AppData\Local\atom\app-1.30.0\resources\app\apm\node_modules\npm\bin\npm-cli.js" "--globalconfig" "C:\Users\user\.atom\.apm\.apmrc" "--userconfig" "C:\Users\user\.atom\.apmrc" "install" "C:\Users\user\AppData\Local\Temp\…\package.tgz" "--runtime=electron" "--target=2.0.5" "--arch=x64" "--global-style"
npm ERR! node v6.9.5
npm ERR! npm v3.10.10
npm ERR! code EREADFILE

npm ERR! Error extracting C:\Users\user.atom.apm\marko\4.13.3\package.tgz archive: ENOENT: no such file or directory, open 'C:\Users\user.atom.apm\marko\4.13.3\package.tgz'
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR! https://github.com/npm/npm/issues

npm ERR! Please include the following file with any support request:
npm ERR! C:\Users\user\AppData\Local\Temp\…\npm-debug.log

(grumble grumple) Now I'm in a pickle. Maybe it's because today I upgraded Node to v10.9.0. Whatever the case, Node won't let me downgrade.

Any ideas? Now I don't even have any formatting plugin in Atom.