incorrectly wrapping in HTML lists #1008

garretwilson · 2016-06-08T23:23:06Z

If I have the following HTML code:

<!DOCTYPE html>
<html>

<head>
    <meta charset="UTF-8" />
    <title>span test</title>
</head>

<body>
    <ul>
        <li><span>foo</span></li>
        <li><span>bar</span></li>
    </ul>
</body>

</html>

Then atom-beautify leaves it just the way I have it. However if I use  instead of :

<!DOCTYPE html>
<html>

<head>
    <meta charset="UTF-8" />
    <title>mark test</title>
</head>

<body>
    <ul>
        <li><mark>foo</mark></li>
        <li><mark>bar</mark></li>
    </ul>
</body>

</html>

Then atom-beautify incorrectly adds newlines:

<!DOCTYPE html>
<html>

<head>
    <meta charset="UTF-8" />
    <title>mark test</title>
</head>

<body>
    <ul>
        <li>
            <mark>foo</mark>
        </li>
        <li>
            <mark>bar</mark>
        </li>
    </ul>
</body>

</html>

This is my complete .jsbeautifyrc file:

{
    "indent_with_tabs": true,
    "preserve_newlines": true,
    "max_preserve_newlines": 2,
    "end_with_newline": true,
    "wrap_line_length": 0
}

atom-beautify should treat  identically to . They are both inline phrase elements.

It turns out this was originally a problem with js-beautify not providing a correct list of inline elements for the unformatted optional, which I reported in beautifier/js-beautify#840 and which js-beautify fixed in version 1.6.0, as @bitwiseman noted when I filed this same issue as beautifier/js-beautify#938. (Technically these should not be "unformatted" but "formatted as inline", but that is a larger issue with the js-beautify and not directly related to the current problem.)

Now it appears (see the Gist ) that for some reason atom-beautify is deciding to send its own list of unformatted elements. I don't know where this list is coming from, but they resemble the old, outdated list that js-beautify used to use.

Debug

Here is a link to the debug.md Gist: https://gist.github.com/garretwilson/1ceab409405c91630033c1fe3c33cad1

Checklist

I have tried uninstalling and reinstalling Atom Beautify to ensure it installed properly
I have reloaded (or restarted) Atom to ensure it is not a caching issue
Searched for existing Atom Beautify Issues at https://github.com/Glavin001/atom-beautify/issues
so I know this is not a duplicate issue
Generated debugging information and added link for debug.md Gist to this issue

The text was updated successfully, but these errors were encountered:

garretwilson · 2016-06-24T15:27:59Z

Any update on this? It would appear from the debug information I submitted that atom-beautify is submitting its own list of unformatted elements without my asking it to, overriding the defaults of js-beautify.

Glavin001 · 2016-06-24T18:19:00Z

atom-beautify is deciding to send its own list of unformatted elements. I don't know where this list is coming from, but they resemble the old, outdated list that js-beautify used to use.

Good eye. Atom-Beautify requires default values and for html.unformatted it was pulled from js-beautify: https://github.com/beautify-web/js-beautify/blob/57b89fcde24199b18dcbcb247c158420d79039dc/js/lib/beautify-html.js#L119-L131
I will merge a Pull Request updating the unformatted list found at https://github.com/Glavin001/atom-beautify/blob/master/src/languages/html.coffee#L74-L78
Thanks!

garretwilson · 2016-06-24T19:12:51Z

Thank you for agreeing to address this, @Glavin001 .

Has atom-beautify always "require[d] default values"? Because I could have sworn that once I was successful in getting someone to fix js-beautify, it worked for a while in atom-beautify.

In a perfect world, atom-beautify wouldn't need default values---if no values were supplied for an option, it would simply let js-beautify use its own defaults.

Glavin001 · 2016-06-24T21:21:12Z

Thank you for agreeing to address this, @Glavin001 .

No problem! I definitely want the default experience to be as expected for developers with experience in that language. Let me know if you have any questions regarding submitting a Pull Request with the appropriate changes to unformatted option.

Has atom-beautify always "require[d] default values"? Because I could have sworn that once I was successful in getting someone to fix js-beautify, it worked for a while in atom-beautify.

Hmm, I cannot recall 100%. Atom-Beautify may not have been so strict in the past.

In a perfect world, atom-beautify wouldn't need default values---if no values were supplied for an option, it would simply let js-beautify use its own defaults.

However, one advantage of having defaults in Atom-Beautify is it enforces defaults between different beautifiers for the same language. Changing from js-beautify to pretty-diff beautifier may act unexpectedly because the default values are different (no concrete example, just postulating). Those defaults may be opinionated and defined by the maintainers. Atom-Beautify will be open to having unified default values that are external to any individual third-party beautifier.

In the case where Atom-Beautify is behind / out-of-date then at least Atom-Beautify is consistent and explicit with its options passed to beautifers and the user can, of course, easily configure Atom-Beautify correctly or submit a Pull-Request such that other users have the improved experience.

garretwilson · 2016-07-26T13:24:10Z

Once again thanks for getting this fix in, @Glavin001 . But I'm a little surprised you scheduled it so far in the future. It fixed a bug that was downright incorrect both in terms of HTML5 and for compatibility with js-beautify. Bug fixes can go into patch releases even under semantic versioning.

Glavin001 · 2016-07-26T14:01:21Z

I think there is some confusion 😜 .

Once again thanks for getting this fix in, @Glavin001 .

What I meant with my comment:

I will merge a Pull Request updating the unformatted list found at https://github.com/Glavin001/atom-beautify/blob/master/src/languages/html.coffee#L74-L78

Is that if someone, such as yourself, submits a Pull Request I would be happy to review and merge it. As always, my only requirement is passing Travis CI tests.
I encourage others to contribute as I am often unavailable because of work, school, and/or family commitments to make changes. So submitting a Pull request for even small issues are a great way to get familiar with Atom-Beautify and contributing.

But I'm a little surprised you scheduled it so far in the future.
...
Bug fixes can go into patch releases even under semantic versioning.

I think you are referring to the fact that v0.30.0 milestone is assigned to this issue. I do not create milestones for patch releases, as they usually are created after I merge a few bug fixing Pull Requests and need to make a quick release. This can definitely be released as a patch before the v0.30.0 release, which is what I meant by assigning that milestone.

Hope that clears things up! 😄

garretwilson · 2016-07-26T14:06:33Z

Ack! So no work has been done on this whatsoever?? But this is a blatant bug---it's not as if atom-beautify did something incorrect---in my opinion it did something completely wrong by simply doing anything at all!! It shouldn't be providing a default value here---it should be letting js-beautify provide a default. If it simply left things alone, then everything would work---and we wouldn't have to be fixing things in two plugins.

Yes, you told me your point of view, which disagrees with mine---which is fine, if I don't have to do any work to get this things unbroken. But I thought you meant you were fixing it.

If I have to do work... I can fix it by submitting a pull request that takes out the default and delegates to js-beautify so we won't have this problem in the future, even when atom-beautify is updated. (I don't like to do work over and over for no reason.) Is that acceptable?

OK---I'm going to have breakfast before I get grumpy. haha Have a great day, @Glavin001 .

Glavin001 · 2016-07-26T14:21:08Z

No idea why you're getting grumpy. I agreed that the default value needs to be updated to match JS-Beautify which aligns with what a profession expects the standards to be.

So no work has been done on this whatsoever??

Nor have you submitted a fix for this? You are equally capable to do so and I have even provided links to applicable files to change.

Regardless, I've made the change and submitted a very simple Pull Request to #1092 and await to see that the Travis CI tests pass. I was hoping you'd like to contribute to Atom-Beautify instead of complaining and being grumpy, especially since you've already contributed before with https://github.com/Glavin001/atom-beautify/commits?author=garretwilson
If this issue is important to you, then work with the community and help contribute changes.

Hope your day cheers you up.

garretwilson · 2016-07-26T14:24:26Z

No idea why you're getting grumpy.

I'm not---I was just explaining why it sounded that way because I haven't had breakfast yet. I'm actually having an awesome day.

I'm happy to contribute to atom-beautify! You didn't give me time to finish breakfast... 😜

garretwilson · 2016-07-26T14:59:26Z

P.S. I probably would have made a contribution to this bug already, but I had understood from your comment that you already had created a pull request that addresses this. Sorry I misunderstood you.

Close #1008. Update HTML.unformatted default value to match js-beautify

garretwilson · 2016-08-14T16:42:42Z

... I've made the change and submitted a very simple Pull Request ...

Since it's been over a week, let me just verify that I didn't yet again misunderstand you: are you waiting on me for anything before this bug fix is released?

garretwilson · 2016-08-24T17:27:12Z

Um... @Glavin001 ? Anything stopping this from being published? Perhaps even a 0.29.1 release, since this is a bug fix?

Glavin001 · 2016-08-24T19:02:56Z

I'm waiting on some Pull Requests that are close to merging and also I've been away on family vacation for few weeks. If the Pull Requests are not ready to merge by tonight when I'm home I will publish 👍 .

garretwilson · 2016-08-24T19:20:09Z

Great, thanks for the update! Don't rush your vacation---I was just checking in for a status. 😃

Glavin001 · 2016-08-26T04:36:19Z

Published to v0.29.11

garretwilson · 2016-08-26T05:05:14Z

Woo hoo! It works!!!! Thank you soooo much! One less irritation in my life...

prettydiff added the third-party-bug label Jun 9, 2016

prettydiff added enhancement and removed third-party-bug labels Jun 24, 2016

Glavin001 added update-dependency and removed update-dependency labels Jun 24, 2016

Glavin001 added this to the v0.30.0 milestone Jun 24, 2016

Glavin001 added a commit that referenced this issue Jul 26, 2016

Close #1008. Update HTML.unformatted default value to match js-beautify

c7178fe

Glavin001 mentioned this issue Jul 26, 2016

Close #1008. Update HTML.unformatted default value to match js-beautify #1092

Merged

4 tasks

Glavin001 closed this as completed Jul 26, 2016

Glavin001 self-assigned this Jul 26, 2016

Glavin001 added the pending-publication label Jul 26, 2016

Glavin001 added a commit that referenced this issue Aug 5, 2016

Merge pull request #1092 from Glavin001/Glavin001-patch-1

9b3bf1b

Close #1008. Update HTML.unformatted default value to match js-beautify

Glavin001 added published and removed pending-publication labels Aug 26, 2016

garretwilson mentioned this issue Jun 25, 2018

not correctly joining lines for HTML beautifier/js-beautify#1033

Closed

garretwilson mentioned this issue Jul 9, 2018

Remove unformatted defaults as per js-beautify bug fix. #2180

Closed

This was referenced Aug 27, 2018

Remove unformatted defaults as per bug fix released in js-beautify 1.8.0. #2210

Closed

Publish v1.8.0 beautifier/js-beautify#1495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incorrectly wrapping <mark> in HTML lists #1008

incorrectly wrapping <mark> in HTML lists #1008

garretwilson commented Jun 8, 2016

garretwilson commented Jun 24, 2016

Glavin001 commented Jun 24, 2016

garretwilson commented Jun 24, 2016

Glavin001 commented Jun 24, 2016

garretwilson commented Jul 26, 2016 •

edited

Loading

Glavin001 commented Jul 26, 2016 •

edited

Loading

garretwilson commented Jul 26, 2016

Glavin001 commented Jul 26, 2016 •

edited

Loading

garretwilson commented Jul 26, 2016

garretwilson commented Jul 26, 2016

garretwilson commented Aug 14, 2016

garretwilson commented Aug 24, 2016

Glavin001 commented Aug 24, 2016

garretwilson commented Aug 24, 2016

Glavin001 commented Aug 26, 2016

garretwilson commented Aug 26, 2016

incorrectly wrapping <mark> in HTML lists #1008

incorrectly wrapping <mark> in HTML lists #1008

Comments

garretwilson commented Jun 8, 2016

Debug

Checklist

garretwilson commented Jun 24, 2016

Glavin001 commented Jun 24, 2016

garretwilson commented Jun 24, 2016

Glavin001 commented Jun 24, 2016

garretwilson commented Jul 26, 2016 • edited Loading

Glavin001 commented Jul 26, 2016 • edited Loading

garretwilson commented Jul 26, 2016

Glavin001 commented Jul 26, 2016 • edited Loading

garretwilson commented Jul 26, 2016

garretwilson commented Jul 26, 2016

garretwilson commented Aug 14, 2016

garretwilson commented Aug 24, 2016

Glavin001 commented Aug 24, 2016

garretwilson commented Aug 24, 2016

Glavin001 commented Aug 26, 2016

garretwilson commented Aug 26, 2016

garretwilson commented Jul 26, 2016 •

edited

Loading

Glavin001 commented Jul 26, 2016 •

edited

Loading

Glavin001 commented Jul 26, 2016 •

edited

Loading