Enable prettier

[timja]

Ref https://github.com/jenkinsci/jenkins/pull/5916#issuecomment-1186117735

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

[NotMyFault]

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]

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)

[github-actions[bot]]

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

[timja]

cc @jenkinsci/sig-ux

[NotMyFault]

The CI build seems to fail on Windows.

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

[timja]

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]

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]

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[bot]]

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

[Wadeck]

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

I think this is pretty valuable feedback for this PR.

---

If this is accepted

Nowadays I have the feeling the first proposal is mainly accepted, without having market consideration / general discussion / etc. It's good in terms of reactivity, but I am not sure about the community involvement / desire. Now, it's a gut feeling, I would like to be proven wrong :D

This part of the comment is mainly to say that this kind of choice must be discussed, agreed and not just merged as a regular typo.

My opinion on Prettier: it's too opiniated (perhaps just by default).

Examples of some forced changes that are not really improving the readability from my PoV

• https://github.com/jenkinsci/jenkins/pull/6863/files#diff-2d80eeff320babaccf0facc6aff210318c479d6c6c44810b16677774a69e1bf3R3

• https://github.com/jenkinsci/jenkins/pull/6863/files#diff-9b871e03a2ce1679fef24d588ec5e18c0e211902f6287f79520368cae4d27181R12

• Long lines are not appreciated at all

In general I like to have such formatters in place, to standardize the code, but it should be not reducing readability.

---

Such changes have a significant cost for the security team when we are working on backports (as well as for the release manager). So, I would like to ensure the community is aligned on the desire and there is a consensus about the rules. I don't want to see such massive reformatting to be done multiple times.

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

Please do, otherwise it's a mess when we are trying to do some git archeology :)

[basil]

Such changes have a significant cost for the security team when we are working on backports (as well as for the release manager).

Did we not have a similar discussion when sorting the imports? My standard advice for doing formatting changes in a multibranch project is to backport the formatting changes to all active branches. This allows commits to be cherry-picked from the main branch onto stable branches with the same ease as prior to the formatting change.

[jglick]

backport the formatting changes

Meaning, cherry-picking the commit or portion of a commit introducing the formatting tool, or cherry-picking the whole change while discarding any changes to source files with conflicts, and then rerunning the tool in apply mode?

[basil]

Right, this PR is really two logical portions, a portion adding new infrastructure, and another portion to apply the formatting onto the rest of the codebase. So for a backport you would logically take the first portion but not the second, then run the apply step onto the backport branch which would obviously yield different results from applying it onto the main branch.

[timja]

I think this is pretty valuable feedback for this PR.

Which was addressed by merging Daniel's 20 or so open PRs in this area.

Long lines are not appreciated at all

That goes over the 80 character line length which is fairly standard for linting tools as a default There is an option there if people feel strongly: https://prettier.io/docs/en/options.html#print-width

The new version there reads fine to me.

addressed by adding braces which fixed the formatting

🤷 not a big issue.

Such changes have a significant cost for the security team when we are working on backports (as well as for the release manager). So, I would like to ensure the community is aligned on the desire and there is a consensus about the rules. I don't want to see such massive reformatting to be done multiple times.

I've split it into 2 commits as suggested by Basil in https://github.com/jenkinsci/jenkins/pull/6863#issuecomment-1202810204 which should allow applying to all active branches

[basil]

I think Wadeck's comment about the ugly else statement could be addresesd by turning on ESLint's curly check in all mode (require braces for all if/else statements). This could be done in a separate PR and merged in as a prerequisite to this PR. If that is done, and if this PR is rebased against that one, then this would systematically eliminate Wadeck's concern about the ugly else statement.

[timja]

This could be done in a separate PR and merged in as a prerequisite to this PR

sure, unfortunately non-trivial as eslint isn't currently ran across that sub-module, but working on it :)

[github-actions[bot]]

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

[kmartens27]

I was reviewing the Prettier tool, and had a few questions in regards to the documentation of Jenkins:

Will having prettier enabled affect current and future documentation updates throughout the ci repo?

Would this go through the .adoc files as well (there are some pages that use HTML or other code within their content so this seems unclear)?

Is this something that we would need to adapt to for documentation contributions?

Does it take into consideration things like accessibility if it does affect the documentation?

Thanks!

[timja]

Will having prettier enabled affect current and future documentation updates throughout the ci repo?

It may cause a minor linting issue which we can review as we go and disable for markdown if it's too onerous.

Would this go through the .adoc files as well (there are some pages that use HTML or other code within their content so this seems unclear)?

No, prettier only supports markdown not adoc. There are only two adoc files in this repository https://github.com/jenkinsci/jenkins/blob/master/docs/MAINTAINERS.adoc and some test file.

Is this something that we would need to adapt to for documentation contributions?

What sort of adaptations were you thinking of? i.e. the changes in this PR to markdown files are quite small. Mostly changing * to -

diff

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 8573531426..6c57cb3b9b 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -9,19 +9,19 @@ This page provides information about contributing code to the Jenkins core codeb
 1. Fork the repository on GitHub
 2. Clone the forked repository to your machine
 3. Install the necessary development tools. In order to develop Jenkins, you need the following:
-   * Java Development Kit (JDK) 11 or 17.
+   - Java Development Kit (JDK) 11 or 17.
      In the Jenkins project we usually use [Eclipse Temurin](https://adoptium.net/) or [OpenJDK](https://openjdk.java.net/), but you can use other JDKs as well.
-   * Apache Maven 3.8.1 or above. You can [download Maven here](https://maven.apache.org/download.cgi).
+   - Apache Maven 3.8.1 or above. You can [download Maven here](https://maven.apache.org/download.cgi).
      In the Jenkins project we usually use the most recent Maven release.
-   * Any IDE which supports importing Maven projects.
-   * Install [NodeJS 16.x](https://nodejs.org/en/). **Note:** only needed to work on the frontend assets found in the `war` module.
-     * Frontend tasks are run using [yarn](https://yarnpkg.com/). Run `npm install -g yarn` to install it.
+   - Any IDE which supports importing Maven projects.
+   - Install [NodeJS 16.x](https://nodejs.org/en/). **Note:** only needed to work on the frontend assets found in the `war` module.
+     - Frontend tasks are run using [yarn](https://yarnpkg.com/). Run `npm install -g yarn` to install it.
 4. Set up your development environment as described in [Preparing for Plugin Development](https://www.jenkins.io/doc/developer/tutorial/prepare/)
 
 If you want to contribute to Jenkins, or just learn about the project,
 you can start by fixing some easier issues.
 In the Jenkins issue tracker we mark such issues as `newbie-friendly`.
-You can find them by using this query (check the link) for [newbie friendly issues](https://issues.jenkins.io/issues/?jql=project%20%3D%20JENKINS%20AND%20status%20in%20(Open%2C%20%22In%20Progress%22%2C%20Reopened)%20AND%20component%20%3D%20core%20AND%20labels%20in%20(newbie-friendly)).
+You can find them by using this query (check the link) for [newbie friendly issues](<https://issues.jenkins.io/issues/?jql=project%20%3D%20JENKINS%20AND%20status%20in%20(Open%2C%20%22In%20Progress%22%2C%20Reopened)%20AND%20component%20%3D%20core%20AND%20labels%20in%20(newbie-friendly)>).
 
 ## Building and Debugging
 
@@ -53,11 +53,13 @@ mvn -pl war jetty:run
 To work on the `war` module frontend assets, two processes are needed at the same time:
 
 On one terminal, start a development server that will not process frontend assets:
+
 ```sh
 mvn -pl war jetty:run -Dskip.yarn

On another terminal, move to the war folder and start a webpack dev server: +

cd war; yarn start

@@ -82,9 +84,9 @@ so there is no strict need to run full test suites before proposing a pull reque

There are 3 profiles for tests:

-* light-test - runs only unit tests, no functional tests -* smoke-test - runs unit tests + a number of functional tests -* all-tests - runs all tests, with re-run (default) +- light-test - runs only unit tests, no functional tests +- smoke-test - runs unit tests + a number of functional tests +- all-tests - runs all tests, with re-run (default)

In addition to the included tests, you can also find extra integration and UI tests in the Acceptance Test Harness (ATH) repository. @@ -93,6 +95,7 @@ If you propose complex UI changes, you should create new ATH tests for them.

JavaScript unit tests

In case there's only need to run the JS tests: +

cd war; yarn test

@@ -105,17 +108,17 @@ All proposed changes are submitted, and code reviewed, using a [GitHub pull requ To submit a pull request:

1. Commit your changes and push them to your fork on GitHub. -It is a good practice is to create branches instead of pushing to master.

• It is a good practice is to create branches instead of pushing to master.

2. In the GitHub Web UI, click the New Pull Request button.
3. Select jenkinsci as base fork and master as base, then click Create Pull Request.

• • We integrate all changes into the master branch towards the Weekly releases.
• • After that, the changes may be backported to the current LTS baseline by the LTS Team.

• • We integrate all changes into the master branch towards the Weekly releases.
• • After that, the changes may be backported to the current LTS baseline by the LTS Team. Read more about the backporting process.

4. Fill in the Pull Request description according to the proposed template.
5. Click Create Pull Request.
6. Wait for CI results/reviews, process the feedback.

• • If you do not get feedback after 3 days, feel free to ping @jenkinsci/core-pr-reviewers in the comments.
• • Usually we merge pull requests after 2 approvals from reviewers, no requested changes, and having waited some more time to give others an opportunity to provide their feedback.

• • If you do not get feedback after 3 days, feel free to ping @jenkinsci/core-pr-reviewers in the comments.
• • Usually we merge pull requests after 2 approvals from reviewers, no requested changes, and having waited some more time to give others an opportunity to provide their feedback. See this page for more information about our review process.

Once your Pull Request is ready to be merged, @@ -130,26 +133,26 @@ The complete list of labels can be found at https://github.com/jenkinsci/jenkins These labels are defined as follows:

• needs-docs marks a pull request as lacking documentation, either for developers (e.g., Javadoc) or users (e.g., changes to the Jenkins handbook). -For such pull requests to be approved and merged, the corresponding changes to the documentation should be proposed. -If those changes belong to a separate repository (e.g., jenkins-infra/jenkins.io), a secondary pull request should be created in draft state in the other repository and reviewed in tandem with the primary pull request that proposes the code change.

• For such pull requests to be approved and merged, the corresponding changes to the documentation should be proposed.
• If those changes belong to a separate repository (e.g., jenkins-infra/jenkins.io), a secondary pull request should be created in draft state in the other repository and reviewed in tandem with the primary pull request that proposes the code change.

• needs-fix marks a pull request which has pending requests for change that have not yet been addressed. -Such pull requests will not be merged until the code has been fixed and the tests pass.

• Such pull requests will not be merged until the code has been fixed and the tests pass.

• needs-justification marks a pull request where the reasoning is unclear, incomplete or not entirely cogent. -To properly evaluate the solution provided in a pull request, maintainers must be able to understand the high-level problem that the pull request attempts to solve. -While the context might be obvious to the author, it is not always apparent to reviewers and maintainers. -The use of design documents, high-level tracking epics, minimal reproducible examples (MREs), etc. is strongly encouraged.

• To properly evaluate the solution provided in a pull request, maintainers must be able to understand the high-level problem that the pull request attempts to solve.
• While the context might be obvious to the author, it is not always apparent to reviewers and maintainers.
• The use of design documents, high-level tracking epics, minimal reproducible examples (MREs), etc. is strongly encouraged.

• needs-more-review marks a pull request as lacking a sufficient number of reviews from subject-matter expert(s) (SME), either because the changes are complex and not sufficiently explained or because there is a lack of consensus regarding the proposed solution.
• on-hold marks a pull request that depends on another event and cannot be merged until the completion of that event. -When the dependent task has been completed, the pull request will be ready for merge.

• When the dependent task has been completed, the pull request will be ready for merge.

• proposed-for-close marks a pull request where there is either no consensus on the next steps or where the next steps have not been taken and an extended period of time has elapsed. -Such pull requests are typically closed approximately one week after the label has been applied. -They can always be reopened once consensus has been reached on the next steps or when action is taken regarding these next steps.

• Such pull requests are typically closed approximately one week after the label has been applied.
• They can always be reopened once consensus has been reached on the next steps or when action is taken regarding these next steps.

• ready-for-merge marks a pull request that has met the acceptance criteria, as defined elsewhere in this document. -If there is no negative feedback, such pull requests are typically merged within approximately 24 hours.

• If there is no negative feedback, such pull requests are typically merged within approximately 24 hours.

• stalled marks a pull request that is off to a promising start but requires additional effort to reach completion - effort that appears to have been abandoned. -If the original author lacks the time and interest to continue the original effort, we suggest that someone else pick up where the original author left off to drive the effort to completion.

• If the original author lacks the time and interest to continue the original effort, we suggest that someone else pick up where the original author left off to drive the effort to completion.

• work-in-progress marks a pull request that remains under active development. -Such pull requests are not ready for final review.

• Such pull requests are not ready for final review.

To ensure that pull requests are processed efficiently, the ready-for-merge, stalled, and proposed-for-close labels are subject to time constraints.

@@ -172,7 +175,7 @@ The setting can be found in Settings -> Editor -> General -> On Save -> Remove t This will help minimize the diff, which makes reviewing PRs easier.

We also do not recommend * imports in the production code. -Please disable them in Settings > Editor > Codestyle > Java by setting Class count to use import with '*' and Names count to use import with '*'_ to a high value, e.g. 100. +Please disable them in Settings > Editor > Codestyle > Java by setting Class count to use import with '*' and Names count to use import with '*'_ to a high value, e.g. 100.

The addition of @{jenkins.addOpens} to argLine exposes a bug in IntelliJ IDEA. A patch has been proposed in JetBrains/intellij-community#1976. @@ -209,7 +212,7 @@ just submit a pull request.

Links

-* Jenkins Contribution Landing Page -* Jenkins Chat Channels -* Beginners Guide To Contributing -* List of newbie-friendly issues in the core +- Jenkins Contribution Landing Page +- Jenkins Chat Channels +- Beginners Guide To Contributing +- List of newbie-friendly issues in the core diff --git a/README.md b/README.md index 3ee3b868bd..529c8234c4 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,8 @@

-In a nutshell, Jenkins is the leading open-source automation server. -Built with Java, it provides over 1,800 plugins to support automating virtually anything, +In a nutshell, Jenkins is the leading open-source automation server. +Built with Java, it provides over 1,800 plugins to support automating virtually anything, so that humans can spend their time doing things machines cannot.

What to Use Jenkins for and When to Use It

@@ -34,17 +34,17 @@ See the Downloads page for references.

For all distributions Jenkins offers two release lines:

-* Weekly - +- Weekly - Frequent releases which include all new features, improvements, and bug fixes. -* Long-Term Support (LTS) - +- Long-Term Support (LTS) - Older release line which gets periodically updated via bug fix backports.

Latest releases:

Source

Our latest and greatest source of Jenkins can be found on GitHub. Fork us!

Contributing to Jenkins

@@ -71,4 +71,5 @@ Jenkins is used by millions of users and thousands of companies. See adopters for the list of Jenkins adopters and their success stories.

License

Jenkins is licensed under the MIT License.

</details>

> Does it take into consideration things like accessibility if it does affect the documentation?

see previous point, changes are trivial and does not affect accessibility.

[kmartens27]

Thanks @timja for the thorough explanation and answering my questions. I appreciate the amount of insight you were able to share. I think that covers all my questions, and since it will not be affecting the .adoc files, that was my biggest concern. Thanks so much!!!

[jamietanna]

Have we considered maybe using something like https://github.com/diffplug/spotless, at least as a JVM managed way to configure it, but also that we can use for other formatting in the future? Or would that be more complex than pure prettier?

Also because Spotless can auto-fix which is super handy!

[timja]

Have we considered maybe using something like diffplug/spotless, at least as a JVM managed way to configure it, but also that we can use for other formatting in the future? Or would that be more complex than pure prettier?

We already have spotless for some other things, it may be technically possibly to configure spotless with prettier here, (https://github.com/diffplug/spotless/tree/main/plugin-maven#prettier) but I'm not sure if it would work given we use yarn v3 with pnp and whether any assumptions in the plugin would hold true, (possibly it wouldn't conflict).

Also because Spotless can auto-fix which is super handy!

the main way to fix prettier is by running prettier --write $path / configuring your IDE to do it.

[daniel-beck]

Have we considered maybe using something like https://github.com/diffplug/spotless, at least as a JVM managed way to configure it, but also that we can use for other formatting in the future? Or would that be more complex than pure prettier?

Also because Spotless can auto-fix which is super handy!

Could you be more specific? There's already https://github.com/jenkinsci/jenkins/blob/799fea814bbbe38710fd47b5408dec9b2295c35f/pom.xml#L424-L458

[jamietanna]

Ah sorry didn't spot we already were using it in some places! Yeah that's fair if it may not directly work for our usecase

[timja]

At a minimum integrating it with spotless would complicate updating it as the version of a JavaScript dependency would be encoded in a pom.xml file, (do-able but more effort)

[basil]

From the Spotless documentation:

Prettier is based on NodeJS, so to use it, a working NodeJS installation (especially npm) is required on the host running spotless. Spotless will try to auto-discover an npm installation. If that is not working for you, it is possible to directly configure the npm binary to use.

This repository does not currently depend on working Node and/or NPM installations, as we use Frontend Maven Plugin in the war/ module to fetch and install Node and Yarn (both classic and Yarn Berry!) as part of the build process. Integrating Spotless Maven Plugin with this system seems complex and difficult, and I am not sure the benefit is worth the cost. In other words, I am not against implementing this idea, but I would not consider that to be blocking this PR.

[basil]

BTW I don't care how many PRs we use, but I feel strongly that we have at least two commits after all is integrated (one for the manual changes to add the braces and one for the automatic changes to apply Prettier), if not three (the above two, plus the commit to add the Prettier infrastructure). Whether those take the form of two (squashed) PRs or one (not squashed) PR makes no difference to me.

[timja]

BTW I don't care how many PRs we use, but I feel strongly that we have at least two commits after all is integrated (one for the manual changes to add the braces and one for the automatic changes to apply Prettier), if not three (the above two, plus the commit to add the Prettier infrastructure). Whether those take the form of two (squashed) PRs or one (not squashed) PR makes no difference to me.

I plan to continue the 3 split commits across any rebasing, I drop the last commit whenever I rebase and just reapply the automated fix. (and to not squash on on merge)

[jglick]

problematic to merge

There is a trick to updating the base branch. You first merge in the commit which enables formatting (but which does not do it), which is unlikely to have any merge conflicts, or at least no more than usual. Then you apply the reformatter to your PR checkout and commit that. Then you merge in master.

[DuMaM]

problematic to merge

There is a trick to updating the base branch. You first merge in the commit which enables formatting (but which does not do it), which is unlikely to have any merge conflicts, or at least no more than usual. Then you apply the reformatter to your PR checkout and commit that. Then you merge in master.

I wasn't aware of this git ~feature~ hack 😉 thanks good to know.