Update html.coffee with js-beautify v1.8.1 #2215

bitwiseman · 2018-09-01T04:49:05Z

What does this implement/fix? Explain your changes.

Adds settings from js-beautify v1.8.1

Does this close any currently open issues?

Closes #2210

Any other comments?

Unsure if this is sufficient.

Checklist

Check all those that are applicable and complete.

Merged with latest master branch
Regenerate documentation with npm run docs
Add change details to CHANGELOG.md under "Next" section?
Added examples for testing to examples/ directory
Travis CI passes (Mac support)
AppVeyor passes (Windows support)

Glavin001 · 2018-09-01T15:18:38Z

src/languages/html.coffee

+      description: "List of tags whose contents should not be reformatted"
+    unformatted:
+      type: 'array'
+      default: []


My only concern is breaking changes for current users using unformatted.

Could you explain the difference between inline, content_unformatted, and old unformatted option?
It sounds like: unformatted has been:

renamed to inline within js-beautify, and

updated the array of tag names

No, inline is an entire new behavior. Unformatted should generally behave the same, but inline more accurately model correct HTML behavior. You have sample tests... But I can make sense of which ones should be failing or not...

Sounds good. Could we leave the existing unformatted without changes then? Then this PR becomes a new feature (adding options) instead of a breaking change of unformatted for those users who do actually use the default value (not recommended).

This PR would add the options:

inline

content_unformatted

Tasks:

languages (i.e. HTML)

atom-beautify/src/languages/html.coffee

Line 67 in fe9c9af

inline:

beautifier (i.e.g js-beautify)

js-beautify is currently using true which means pass all options

https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/js-beautify.coffee#L10

Fix any broken regression tests

I would not expect any large changes, just maybe a few little things because js-beautify is being updated. However, the configurations should stay the same

Add a new test using inline, demonstrating @garretwilson 's problem is resolved

Could we leave the existing unformatted without changes then?

No, please don't do that. That would be partially defeating the purpose of inline, because you'll be saying not to format things like <a> at all!! And no one wants that. We only had it not being unformatted before because it would format it as "not inline". This has been fixed.

… a breaking change of unformatted for those users who do actually use the default value …

But nobody wants the old unformatted default values! Nobody says, "I like my inline elements the way they are; keep your hands off." And if there is that one person, they can add "unformatted" to their settings.

But otherwise, to get inline elements to be formatted, if you leave the existing unformatted settings everyone (the 99.997% of people who want them to be formatted) will have to add "unformatted": [] to their manual settings!!

@Glavin001 I agree with Garret that keeping unformatted will make inline largely ineffective. But in the interest of getting the new options added and available I'm happy to keep the existing default in this version, if that is what it takes. I understand the need to limit the risk of breaking changes.

But otherwise, to get inline elements to be formatted, if you leave the existing unformatted settings everyone (the 99.997% of people who want them to be formatted) will have to add "unformatted": [] to their manual settings!!

@bitwiseman Do you also expect everyone to be using "unformatted": [] from now on? Is there no value in unformatted anymore and going to be removed from js-beautify later? Given the new ideal value for unformatted is [] it sounds like we should remove the unformatted option entirely in favour of inline and content_unformatted which "more accurately model correct HTML behavior".

Fortunately, unformatted is only used by JS Beautify beautifier: https://github.com/Glavin001/atom-beautify/blob/master/docs/options.md#unformatted
So if we really need to make a breaking change, let's do it.
If the expectation is unformatted should no longer be used, let's not only go half way and set it to [], let's remove it entirely and make a big note in the changelog about these new and improved options for formatting.

Glavin001 · 2018-09-01T20:21:05Z

Thanks for submitting this PR, @bitwiseman ! 🎉

bitwiseman · 2018-09-02T20:40:07Z

@Glavin001
I've done as you asked, but there are failing tests. As far as I can tell, they are tests that are also failing on master. Not sure how to address this beyond that.

Also, you requested a test showing the inline behavior is working, but it is hard to make that happen with unformatted still turned on - setting elements to unformatted would stop those elements from behaving like inline elements. 😢 Not sure how to address this either.

Other than that this is ready.

stevenzeck · 2018-09-03T02:18:17Z

@bitwiseman can you please do the following:

Rebase with master
Open examples/nested-jsbeautifyrc/python/expected/test.py and remove the semicolon on line 16
Rename the test.marko file here examples/nested-jsbeautifyrc/marko/original/test.marko to _test.marko

It looks like JS Beautify doesn't format examples/simple-jsbeautifyrc/blade/original/test.blade.php the way it expects to with your change though, so you'll have to look into that.

I'll look into the Linux builds.

bitwiseman · 2018-09-03T02:49:43Z

@szeck87
I got the same error on master.

stevenzeck · 2018-09-03T02:50:52Z

What same error? The blade.php one?

bitwiseman · 2018-09-03T02:51:23Z

Yes. I get the same error in blade on master. :(

bitwiseman · 2018-09-03T02:52:22Z

@szeck87 Ah, but it was before your release an hour ago.

stevenzeck · 2018-09-03T02:58:31Z

The blade.php error is definitely not from master. I'm going through the other errors now. You can actually wait to rebase from master.

stevenzeck · 2018-09-03T03:51:58Z

@bitwiseman looks like two failures:

{{ a_variable }} is moved to the next line in js-beautify 1.8.1. You'll need to change the expected file there to match what js-beautify does now.
Same deal with blade, although more changes are made to those lines.

bitwiseman · 2018-09-04T17:29:05Z

@szeck87
The {{ a_variable }} change is already in my PR.

The blade change: When I put original/test.blade.php into https://beautifier.io I get exactly what is in expected/test.blade.php. I don't understand why it is failing.

stevenzeck · 2018-09-04T17:37:28Z

@bitwiseman I'll take a closer look when I get home. But try running the original using atom beautify with the updated js-beautify version in your PR here. Maybe it's not running the same options as beautifier.io

stevenzeck · 2018-09-04T20:02:44Z

@bitwiseman except for a couple of spaces, js-beautify and beautifier.io both change the blade file in these two places:

This is the {{ $mater }} sidebar. was moved to the next line
Forbidden was moved to the next line

For some reason atom-beautify is putting two spaces after the @foreach and @if statements instead of one. Looks like the Blade block in js-beautify.coffee needs adjusting.

bitwiseman · 2018-09-05T08:11:41Z

@szeck87 Thanks! I think I got it. The blade template was expecting space not to be added to void elements - <br />. I fixed the regex to allow for them.

Glavin001

Looking good! Almost there.

@szeck87 Good to merge and publish, after fixing typos and your approval 👍 .

Thanks!! 🎉

Glavin001 · 2018-09-06T02:39:28Z

src/languages/html.coffee

@@ -79,7 +79,27 @@ module.exports = {
        ]
      items:
        type: 'string'
-      description: "List of tags (defaults to inline) that should not be reformatted"
+      description: "(Depracated for most scenarios) List of tags that should not be reformatted at all.  NOTE: Set this to [] to get improved beautifier behavior."


Typo: Deprecated

Re: Note about improved behaviour.
Do you want to mention inline option at all?

bitwiseman · 2018-09-06T06:27:24Z

@Glavin001

@bitwiseman Do you also expect everyone to be using "unformatted": [] from now on? Is there no value in unformatted anymore and going to be removed from js-beautify later? Given the new ideal value for unformatted is [] it sounds like we should remove the unformatted option entirely in favour of inline and content_unformatted which "more accurately model correct HTML behavior".

No, unformatted is still useful in some cases - it is functionality that people can use to mitigate behavior they don't want. We should totally direct user to the other two options, but I see no reason to remove it.

Fortunately, unformatted is only used by JS Beautify beautifier: https://github.com/Glavin001/atom-beautify/blob/master/docs/options.md#unformatted
So if we really need to make a breaking change, let's do it.
If the expectation is unformatted should no longer be used, let's not only go half way and set it to [], let's remove it entirely and make a big note in the changelog about these new and improved options for formatting.

I do not think we can or should remove unformatted, but I would of course rather have it set to [] as it produces much better behavior. I made the change to keep the values in unformatted as you requested and now you want to change them back to what I originally posted? 😱

A note in the changelog in any case would be a good idea.

Glavin001 · 2018-09-06T16:50:34Z

I made the change to keep the values in unformatted as you requested and now you want to change them back to what I originally posted? 😱

Sorry about that 😝 . I did not think leaving unformatted as it originally was would mean inline is ineffective and most users would be experiencing poor behaviour and be filling their configs with unformatted: [] 🔥 .

I trust your judgement. 👍

Glavin001 · 2018-09-06T16:52:05Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+
+# v0.33.2 (2018-09-09)
+- Added `inline` and `content_unformatted` options from `js-beautify` html settings and cleared `unformatted`.  This is a breaking change, but a general improvement in the behavior to more accurately beautify html.


Could you add a link to this PR and any PRs or Issues from js-beautify? And maybe bold breaking change or do something to make sure users see it 🔥 . Thanks!

Glavin001 · 2018-09-06T16:53:30Z

@szeck87 since you are more actively maintaining Atom-Beautify, would you be able to merge when ready and publish? Thank you 😃 .

Thanks for contributing, @bitwiseman ! 🎉

bitwiseman · 2018-09-11T23:00:23Z

@Glavin001 @szeck87
I went ahead and updated the change log to highlight the breaking change and linked various PRs.

stevenzeck · 2018-09-12T00:59:13Z

Merged

garretwilson · 2018-09-17T23:51:39Z

I saw the bounty claim, @bitwiseman . I'm excited to see this done and will of course go in and approve the payment. I was hoping to see the final release shipped, though, just to see it in action. I guess that's still not out yet.

bitwiseman · 2018-09-18T01:11:50Z

No hurry.

…

On Mon, Sep 17, 2018, 4:51 PM Garret Wilson ***@***.***> wrote: I saw the bounty claim, @bitwiseman <https://github.com/bitwiseman> . I'm excited to see this done and will of course go in and approve the payment. I was hoping to see the final release shipped, though, just to see it in action. I guess that's still not out yet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2215 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3kKYjh4ow1Tu5Iz0i6AEHvUSJ2REeoks5ucDWOgaJpZM4WWEyF> .

garretwilson · 2018-09-18T01:37:06Z

Wait, I see that v0.33.1 was marked above. So was this included in the public release of atom-beautify already?

stevenzeck · 2018-09-18T14:06:56Z

@garretwilson no, I forgot to update the milestones after the v0.33.1 release. It will be included in the next release.

garretwilson · 2018-09-18T18:57:02Z

The suspense is killing me… 😀

garretwilson · 2018-09-26T15:36:17Z

OK… I don't want to be irritating, but you know I've been waiting years for this. 😁 Today I couldn't resist any longer: "Are we there yet?" 😄 i.e. Do we have a release planned soon to include this super-important-to-me bug fix?

garretwilson · 2018-09-26T15:45:42Z

@stevenzeck, after reviewing the messages, I'm not sure why this didn't go out already. @Glavin001 already gave permission above to release this.

I've been waiting years for this big bug to be fixed, and I've even contributed hundreds of dollars in bounty money. At every step there seems to be something impeding this from actually going out the door. Now I don't see what the hold-up is. It's done. It's merged. Everybody has been paid. What needs to be done next to release this thing?

stevenzeck · 2018-09-26T16:19:31Z

@garretwilson we've been focusing on Unibeautify and only doing PR's and releases here as requested. I just released v0.33.2 which includes this.

garretwilson · 2018-09-26T16:25:55Z

@stevenzeck you don't know how much I appreciate this. Good luck with your Unibeautify work. I'm eagerly looking forward to v0.33.2.

bitwiseman force-pushed the patch-2 branch 4 times, most recently from dd9546d to fe9c9af Compare September 1, 2018 05:32

Glavin001 requested review from stevenzeck and Glavin001 September 1, 2018 14:52

Glavin001 requested changes Sep 1, 2018

View reviewed changes

Glavin001 assigned bitwiseman Sep 1, 2018

Glavin001 added high priority add-beautifier-option help wanted in progress labels Sep 1, 2018

bitwiseman force-pushed the patch-2 branch 2 times, most recently from 6e0303b to 4a7558a Compare September 2, 2018 20:32

bitwiseman force-pushed the patch-2 branch from 4a7558a to d7f5c4d Compare September 4, 2018 17:29

stevenzeck mentioned this pull request Sep 4, 2018

Fix CI issues #2221

Merged

6 tasks

bitwiseman added 2 commits September 5, 2018 00:04

Update html.coffee with js-beautify v1.8.1

4a3c093

Update to latest release

e209220

stevenzeck approved these changes Sep 5, 2018

View reviewed changes

Glavin001 requested changes Sep 6, 2018

View reviewed changes

bitwiseman added 3 commits September 5, 2018 23:30

Typo fix

a3accd1

Set unformattted to empty

38bd8b5

Add change log and other message changes

9445822

Glavin001 approved these changes Sep 6, 2018

View reviewed changes

Update changelog to highlight breaking change

f1e6380

bitwiseman changed the title ~~Update html.coffee with js-beautify v1.8.1~~ Update html.coffee with js-beautify v1.8.6 Sep 11, 2018

bitwiseman changed the title ~~Update html.coffee with js-beautify v1.8.6~~ Update html.coffee with js-beautify v1.8.1 Sep 11, 2018

stevenzeck merged commit 67f4ca4 into Glavin001:master Sep 12, 2018

stevenzeck modified the milestones: Next Release, v0.33.1 Sep 12, 2018

bitwiseman deleted the patch-2 branch September 12, 2018 05:19

stevenzeck added the published label Sep 26, 2018

		@@ -1,3 +1,7 @@

		# v0.33.2 (2018-09-09)
		- Added `inline` and `content_unformatted` options from `js-beautify` html settings and cleared `unformatted`. This is a breaking change, but a general improvement in the behavior to more accurately beautify html.

Update html.coffee with js-beautify v1.8.1 #2215

Update html.coffee with js-beautify v1.8.1 #2215

Conversation

bitwiseman commented Sep 1, 2018 • edited Loading

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Glavin001 Sep 1, 2018

Choose a reason for hiding this comment

bitwiseman Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Glavin001 Sep 1, 2018

Choose a reason for hiding this comment

garretwilson Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

bitwiseman Sep 2, 2018

Choose a reason for hiding this comment

Glavin001 Sep 6, 2018

Choose a reason for hiding this comment

Glavin001 commented Sep 1, 2018

bitwiseman commented Sep 2, 2018

stevenzeck commented Sep 3, 2018

bitwiseman commented Sep 3, 2018

stevenzeck commented Sep 3, 2018

bitwiseman commented Sep 3, 2018

bitwiseman commented Sep 3, 2018

stevenzeck commented Sep 3, 2018

stevenzeck commented Sep 3, 2018

bitwiseman commented Sep 4, 2018

stevenzeck commented Sep 4, 2018

stevenzeck commented Sep 4, 2018 • edited Loading

bitwiseman commented Sep 5, 2018

Glavin001 left a comment

Choose a reason for hiding this comment

Glavin001 Sep 6, 2018

Choose a reason for hiding this comment

Glavin001 Sep 6, 2018

Choose a reason for hiding this comment

bitwiseman commented Sep 6, 2018 • edited Loading

Glavin001 commented Sep 6, 2018

Glavin001 Sep 6, 2018

Choose a reason for hiding this comment

Glavin001 commented Sep 6, 2018

bitwiseman commented Sep 11, 2018

stevenzeck commented Sep 12, 2018

garretwilson commented Sep 17, 2018

bitwiseman commented Sep 18, 2018 via email

garretwilson commented Sep 18, 2018

stevenzeck commented Sep 18, 2018

garretwilson commented Sep 18, 2018

garretwilson commented Sep 26, 2018

garretwilson commented Sep 26, 2018 • edited Loading

stevenzeck commented Sep 26, 2018

garretwilson commented Sep 26, 2018

bitwiseman commented Sep 1, 2018 •

edited

Loading

bitwiseman Sep 1, 2018 •

edited

Loading

garretwilson Sep 1, 2018 •

edited

Loading

stevenzeck commented Sep 4, 2018 •

edited

Loading

bitwiseman commented Sep 6, 2018 •

edited

Loading

garretwilson commented Sep 26, 2018 •

edited

Loading