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

Enable prettier #6863

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Enable prettier #6863

merged 3 commits into from
Aug 25, 2022

Conversation

timja
Copy link
Member

@timja timja commented Jul 16, 2022

Ref #5916 (comment)

Automatically lint (and fix) files supported by prettier

  • css
  • JavaScript
  • html
  • markdown
  • yaml

Prettier is an opinionated formatter that can be used to reformat code to match a consistent style.

I've integrated eslint with it so that they won't conflict
Prettier automatically reads the .editorconfig file to read some basic options as well.

Specific sections where the layout produced by prettier is harder to read than what the developer wants can be disabled with https://prettier.io/docs/en/ignore.html

If this is accepted I plan to add a .git-blame-ignore-revs file so this change is ignored in blames.

Interesting files that changed are:

  • war/package.json
  • war/pom.xml
  • war/.prettierignore
  • war/.eslintrc.js
  • CONTRIBUTING.md

@timja timja requested review from a team July 16, 2022 13:45
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Please hold until the number of open PRs is at a minimum.

@timja timja added skip-changelog Should not be shown in the changelog web-ui The PR includes WebUI changes which may need special expertise labels Jul 16, 2022
@NotMyFault
Copy link
Member

How well does that rule out with the recently added integrations like ESLint on ci.j? E.g. in case of a violation found

Do we really need (and want) prettier for markdown and yaml? It looks like it's prefectly fine for frontend files only, though what about handlebars?

@timja
Copy link
Member Author

timja commented Jul 16, 2022

How well does that rule out with the recently added integrations like ESLint on ci.j? E.g. in case of a violation found

Basically: https://prettier.io/docs/en/integrating-with-linters.html

A bit further to this, prettier's opinion is you don't need to know what is wrong, just run the format command and it will fix it for you, so there's no CI view or local tool you need to run it and diff it to see what the changes are.

Do we really need (and want) prettier for markdown and yaml? It looks like it's prefectly fine for frontend files only, though what about handlebars?

🤷 can back it out if we don't want it.

though what about handlebars?

It doesn't support handlebars in the core (https://github.com/timja/jenkins/blob/22022993b29b5498e7ec272a45cd43359951da5c/war/.prettierignore#L24)

I did find a plugin but it seems basically unused so not sure about it (https://github.com/ratierd/prettier-plugin-hbs)

@timja timja force-pushed the enable-prettier branch from 2202299 to 3b7be50 Compare July 16, 2022 14:49
@timja timja dismissed daniel-beck’s stale review July 16, 2022 14:53

no change in this PR requested, I'll mark as on hold for a bit unless others disagree

@timja timja added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jul 16, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 18, 2022
@github-actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@jglick jglick mentioned this pull request Jul 18, 2022
6 tasks
CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
core/src/main/java/hudson/model/listeners/package.html Outdated Show resolved Hide resolved
Comment on lines 4 to 7
(<a
href="https://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html"
>See JDK networking properties for more details</a
>) to enable internet connection, you can specify the HTTP proxy server name
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty unfortunate.

war/pom.xml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 30, 2022
@basil basil removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jul 30, 2022
@timja
Copy link
Member Author

timja commented Jul 30, 2022

cc @jenkinsci/sig-ux

@timja timja requested a review from jglick July 30, 2022 21:20
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

The CI build seems to fail on Windows.

@NotMyFault
Copy link
Member

The CI build seems to fail on Windows.

Windows attempts to enforce CRLF with prettier, but the files use LF, as configured.

@basil basil added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Jul 31, 2022
@timja
Copy link
Member Author

timja commented Aug 1, 2022

The CI build seems to fail on Windows.

Windows attempts to enforce CRLF with prettier, but the files use LF, as configured.

Doesn't seem to do that in general.
I started a windows 11 VM.

Installed java,maven and git
Did no configuration at all.

Lint passed just fine.

Ran out of time but I'll start a VM based off of the CI image later on.

Would ❤️ it if someone on Windows already could take a look.

I've had a look through the prettier issues, as far as I can tell it tries to autodetect line endings and leaves them alone, (it also supports configuring an explicit one).

We have a .gitattributes set for auto eol conversion which I think leaves them alone in the checked out code but will convert on check in.

@NotMyFault
Copy link
Member

NotMyFault commented Aug 1, 2022

The CI build seems to fail on Windows.

Windows attempts to enforce CRLF with prettier, but the files use LF, as configured.

Doesn't seem to do that in general. I started a windows 11 VM.

That was the case when I ran lint:fix. Figures, my surface had autocrlf set to true in its .gitconfig. Unsetting this behavior makes prettier not fail the build.
Running lint:fix again leaves no changed files anymore.

@timja
Copy link
Member Author

timja commented Aug 1, 2022

That was the case when I ran lint:fix. Figures, my surface had autocrlf set to true in its .gitconfig. Unsetting this behavior makes prettier not fail the build.

It seems like that is somewhat recommended for windows:
https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 2, 2022
@timja timja requested a review from a team August 22, 2022 09:46
@timja
Copy link
Member Author

timja commented Aug 22, 2022

(you can ignore merge conflicts they are easily addressed but I don't want to constantly be doing it)

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 23, 2022
@timja timja requested a review from NotMyFault August 23, 2022 07:05
@timja
Copy link
Member Author

timja commented Aug 24, 2022

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 24, 2022
@timja timja merged commit 04b46d1 into jenkinsci:master Aug 25, 2022
@timja timja deleted the enable-prettier branch August 25, 2022 20:51
@NotMyFault NotMyFault removed the request for review from a team August 25, 2022 21:12
@timja
Copy link
Member Author

timja commented Aug 25, 2022

Backported to:

  • stable-2.361
  • stable-2.346
  • stable-2.332
  • stable-2.319

@basil
Copy link
Member

basil commented Aug 25, 2022

Thanks, Tim! I just went through and resolved a bunch of conflicts in open PRs. Could use your help in resolving the logical conflicts in #6511 and #6408.

@daniel-beck
Copy link
Member

Caused JENKINS-69498

@jglick
Copy link
Member

jglick commented Sep 20, 2022

What might have been obvious to the author but took me a long time to figure out, and seems to be missing from any guides: how you actually fix warnings. AFAICT:

mvn -pl war frontend:yarn -Dfrontend.yarn.arguments=lint:fix

@timja
Copy link
Member Author

timja commented Sep 21, 2022

Ah it was in a guide here but got dropped through all the rebasing accidentally I’ve had a PR open for a couple of weeks to automatically fix these on commit:
#7075

@basil
Copy link
Member

basil commented Sep 21, 2022

Those of us who hate Git hooks will never use #7075, so I think it is worth improving the error message and/or documentation to instruct the user about how to fix lint errors using Maven.

@timja timja mentioned this pull request Sep 23, 2022
13 tasks
@basil
Copy link
Member

basil commented Mar 17, 2023

Broke javaposse.jobdsl.plugin.EmbeddedApiDocGeneratorSpec, as discovered in jenkinsci/job-dsl-plugin#1265.

@daniel-beck
Copy link
Member

Caused JENKINS-70788.

@basil
Copy link
Member

basil commented Mar 23, 2023

Broke io.jenkins.plugins.casc.ConfigurationAsCodeTest#testHtmlDocStringRetrieval and io.jenkins.plugins.casc.SchemaGenerationSanitisationTest#testRetrieveDocStringFromAttribute, as discovered in jenkinsci/configuration-as-code-plugin#2234.

daniel-beck added a commit to daniel-beck/jenkins that referenced this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants