Add range ignore support for html languages. by voithos · Pull Request #6693 · prettier/prettier

voithos · 2019-10-22T05:23:46Z

This adds support for the prettier-ignore-start and prettier-ignore-end
comment markers in html based languages, allowing prettier to preserve
existing formatting of multi-element sections of code.

Fixes #5508.

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
(If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

voithos · 2019-10-22T06:20:20Z

This PR has one problem that I can't seem to figure out: the  tag gets auto-indented properly (while its contents don't, of course) but the  tag seems to just take whatever indentation it had prior to formatting. Example.

This adds support for the prettier-ignore-start and prettier-ignore-end comment markers in html based languages, allowing prettier to preserve existing formatting of multi-element sections of code. Fixes prettier#5508.

voithos · 2019-10-23T03:16:28Z

I played around with it a bit more, and made some changes that fix the issue I described earlier! Unfortunately, it feels somewhat hacky, although I can't think of any actual corner cases off the top of my head?

Any thoughts welcome.

lydell · 2019-11-02T09:05:02Z

@ikatyang Since you implemented the HTML printer, and the markdown printer which supports range ignore, could you review this please?

voithos · 2019-11-24T17:48:31Z

Friendly ping?

alexander-akait · 2019-11-25T10:27:24Z

src/language-html/printer-html.js

+   * the child should process normally.
+   * @return {'ignore' | 'continue' | Doc}
+   */
+  function maybeIgnore(path, index, ignoreRanges) {


Let's move this to util

It's a bit difficult to move this as it uses variables from outer scopes and the doc builders. Other functions in utils don't use the doc builders. Let's keep it as is.

src/language-html/printer-html.js

alexander-akait

Can we add strange test like:

<div><!-- prettier-ignore-start --></div><!-- prettier-ignore-end -->
<div><!-- prettier-ignore-start --><span>test</span></div><!-- prettier-ignore-end -->

alanorozco · 2020-03-12T18:20:25Z

👋

Alan here from ampproject/amphtml.

We love prettier, and use it across the board except for HTML files.

Our blocker is that our documents hold something we call the "AMP boilerplate", which is a standard and specific subtree: a <style> element and (sometimes) a <noscript> element. See our playground for a base document.

In our ecosystem this subtree is usually written as one line, which is commonly pasted or automatically inserted in documents.  (for next line) won't work for the <noscript> element, since it's a sibling node.

This change would allow us to use prettier for all our HTML files, as we'd be able to hold the entire boilerplate in one explicit ignore section. This is useful for AMP authors who use prettier too.

Please let us know how we can help to get this merged!

j-f1 · 2020-03-13T00:21:06Z

Could you include a  inside the boilerplate before the noscript and avoid the issue that way?

alanorozco · 2020-03-13T16:38:30Z

@j-f1

Thanks for the suggestion. That can work, but we're avoiding it since we'd still like to keep the boilerplate's ignore section explicitly scoped.

<!-- prettier-ignore-start -->
<style amp-boilerplate>/* ... */</style><noscript><style amp-boilerplate>/* ... */</style></noscript>
<!-- prettier-ignore-end -->

vs

<!-- prettier-ignore -->
<style amp-boilerplate>/* ... */</style>
<!-- prettier-ignore -->
<noscript><style amp-boilerplate>/* ... */</style></noscript>

The boilerplate is often pasted, or new authors might see it and be unsure what it is. I worry that splitting it might create confusion, especially since we "validate" for its presence on every doc.

…e-html

…ore-html

voithos · 2020-03-23T04:43:45Z

Hey, sorry for the radio silence on this, but thanks for making the fixes to this! Let me know if there's anything I can do to help this get merged. 👍

thorn0 · 2020-03-23T10:06:47Z

This PR needs 2 more reviews from the collaborators.

Besides, this is a feature, and there are fixes in master that we might want to publish as a patch release 2.0.2 before merging new features.

alexander-akait

Looks good

/cc @prettier/core

j-f1

Bug:

Prettier pr-6693
Playground link

--parser html

Input:

<div>
  <!-- prettier-ignore-start -->
    <span>

      Test</span><!-- prettier-ignore-end -->


  <!-- prettier-ignore-start -->
    <span>

      Test</span>
  <!-- prettier-ignore-end -->
</div>

Output:

<div>
  <!-- prettier-ignore-start -->
    <span>

      Test</span>><!-- prettier-ignore-end -->

  <!-- prettier-ignore-start -->
    <span>

      Test</span><!-- prettier-ignore-end -->
</div>

thorn0

Bug # 2 (whitespace is lost between the spans):

Prettier pr-6693
Playground link

--parser html

Input:

<div>
<!-- prettier-ignore-start -->
    <span>Test</span>
  <!-- prettier-ignore-end --><span>2</span>
</div>

Output:

<div>
  <!-- prettier-ignore-start -->
    <span>Test</span><!-- prettier-ignore-end --><span
    >2</span
  >
</div>

thorn0 · 2020-05-06T23:59:54Z

@voithos Do you plan to fix the bugs found above? If not, we can probably ask @alanorozco to look into them.

voithos · 2020-05-08T03:41:37Z

Hello! Yep, plan on taking a look soon.

voithos · 2020-07-17T08:34:00Z

Welp, super late response to this, but I still haven't managed to find the time/motivation to pick this up again. :\ Really sorry about that. That being said, feel free to have someone else take a look at fixing the remaining bugs and getting this merged, if the desire is still there. 👍

sosukesuzuki · 2020-07-18T18:40:48Z

@voithos Okay, thank you. Let's keep this PR open, remove from 2.1 milestone. It may be good to put help wanted label.
/cc @thorn0 @fisker @evilebottnawi

camslice · 2024-09-18T17:25:33Z

bump

voithos force-pushed the range-ignore-html branch from 4a773cb to 04e73bf Compare October 22, 2019 05:39

Add range ignore support for html languages.

ba7821b

This adds support for the prettier-ignore-start and prettier-ignore-end comment markers in html based languages, allowing prettier to preserve existing formatting of multi-element sections of code. Fixes prettier#5508.

voithos force-pushed the range-ignore-html branch from 04e73bf to ba7821b Compare October 23, 2019 03:14

alexander-akait reviewed Nov 25, 2019

View reviewed changes

src/language-html/printer-html.js Outdated Show resolved Hide resolved

alexander-akait requested changes Nov 25, 2019

View reviewed changes

alanorozco mentioned this pull request Mar 12, 2020

Range Ignore Support in HTML #5508

Open

Merge remote-tracking branch 'remotes/upstream/next' into range-ignor…

62f7b3a

…e-html

thorn0 changed the base branch from master to next March 17, 2020 01:07

thorn0 added 2 commits March 17, 2020 03:16

move a function to utils

d36fbab

add tests for invalid ignore comments

7b881dd

thorn0 requested review from alexander-akait and j-f1 March 17, 2020 01:25

thorn0 approved these changes Mar 18, 2020

View reviewed changes

thorn0 force-pushed the next branch from 55247de to 262423a Compare March 21, 2020 19:03

thorn0 changed the base branch from next to master March 21, 2020 21:02

thorn0 force-pushed the master branch from 0be5680 to e23e5cc Compare March 22, 2020 01:49

Merge remote-tracking branch 'remotes/upstream/master' into range-ign…

1878d42

…ore-html

thorn0 added this to the 2.1 milestone Mar 23, 2020

thorn0 requested a review from sosukesuzuki March 23, 2020 10:08

alexander-akait approved these changes Mar 23, 2020

View reviewed changes

j-f1 requested changes Mar 24, 2020

View reviewed changes

thorn0 requested changes Mar 24, 2020

View reviewed changes

sosukesuzuki removed this from the 2.1 milestone Jul 18, 2020

fisker added this to the 2.2 milestone Aug 4, 2020

Base automatically changed from master to main January 23, 2021 17:13

thorn0 removed this from the 2.3 milestone Feb 11, 2021

thorn0 added the help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! label Mar 12, 2021

Uh oh!

Conversation

voithos commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voithos commented Oct 22, 2019

Uh oh!

voithos commented Oct 23, 2019

Uh oh!

lydell commented Nov 2, 2019

Uh oh!

voithos commented Nov 24, 2019

Uh oh!

alexander-akait Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

thorn0 Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alanorozco commented Mar 12, 2020

Uh oh!

j-f1 commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanorozco commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voithos commented Mar 23, 2020

Uh oh!

thorn0 commented Mar 23, 2020

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

j-f1 left a comment

Choose a reason for hiding this comment

Uh oh!

thorn0 left a comment

Choose a reason for hiding this comment

Uh oh!

thorn0 commented May 6, 2020

Uh oh!

voithos commented May 8, 2020

Uh oh!

voithos commented Jul 17, 2020

Uh oh!

sosukesuzuki commented Jul 18, 2020

Uh oh!

camslice commented Sep 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

voithos commented Oct 22, 2019 •

edited

Loading

j-f1 commented Mar 13, 2020 •

edited

Loading

alanorozco commented Mar 13, 2020 •

edited

Loading