Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ran prettify over app manager's base templates #35630

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

orangejenny
Copy link
Contributor

Technical Summary

Prep for migrating app manager to webpack

Safety Assurance

Safety story

Automated linting changes

Automated test coverage

no

QA Plan

Not requesting QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 16, 2025
Comment on lines 102 to 103
<i class="fcc fcc-app-createform"></i> Registration Form.{% endblocktrans
%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this type of whitespace changes on the {% blocktrans %} tags is probably what's causing the errors in tests. I'm honestly quite annoyed that Prettier is even trying to format here, but the issue seems to be that it's treating anything inside <script> tags differently from the rest of the file, forcing a different parser than the jinja-template plugin we have set up for HTML file... which then results in different (broken) formatting inside the <script> tags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turning off the embedded-language-formatting option should in theory be the way to fix it, however it is instead resulting in blanking out the entire file (maybe because jinja/django tags look like an "embedded" language).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a somewhat ugly option here, which is using html comments to ignore the sections that use django template tags inside of script tags (because that seems to be what's causing issues). The syntax for this is specific to the parser, so using prettier-plugin-jinja-template it looks like this:

<!-- prettier-ignore-start -->
<script type="text/html">
  ...
</script>
<!-- prettier-ignore-end -->

https://github.com/davidodenwald/prettier-plugin-jinja-template?tab=readme-ov-file#ignoring-code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you for investigating. I'd rather not ignore everything inside of script tags, since they do contain HTML and in theory should be formatted. I played around with this and have a few more thoughts:

  1. Changing the type on the script block from text/html to text/template also seems to tell prettier not to format the contents, so basically a more concise (but less explicit) version of the comments. But I don't like this for the same reason: this content ought to get formatted.
  2. Another option could be to move the template content into a separate file and then include it. If the script tags are in the outer file and only the inner content is in the included file, prettify won't know it's a script block and ought to format it normally.
  3. Yet another option is to increase allowed line length, which doesn't fix this problem directly but generally makes prettify make fewer changes. I think 80 is too short, especially because it's shorter than our python limit. A 115-character limit would have allowed the original code to pass by unmodified.

I read through the full diff (which I had not previously done, but now I know it's important to skim the prettify changes) and made a couple changes in 1957b44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants