I'd like to submit a Jenkins Enhancement Proposal for optional automated source code formatting available from the plugin pom so that plugins which build with maven could choose to "opt-in" to automated source code formatting.
-- Key attributes of the idea:
I've implemented a sample in the platformlabeler plugin using the "FMT Maven plugin" which uses google-java-format to format Java source code to Google Java Style. Key questions that need discussion (especially considering my feeble skills with maven):
A plugin maintainer that chooses to adopt automated source code formatting needs to understand that automated source code formatting creates a "diff wall" due to the many changes made by the formatting process. That is one of the penalties associated with automated source code formatting. Plugins with many open pull requests (git plugin, git client plugin) should not enable automated formatting until they've reduced the number of open pull requests to very few. Otherwise, most open pull requests for the plugin will have many conflicts as soon as the automated formatting is committed. That is another of the penalties of automated source code formatting. Plugins that adopt automated source code formatting will likely find it easier to read the source code and easier to submit clean pull requests. Automated formatting avoids the poor experience of pull request submitters being asked to fix the white space diffs introduced by their pull request. Thanks, Mark Waite You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtExx4tXNbAVCSr85jPDTKR1Zy_BJrxkcp_kJdAfcVaZgQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
The problem I have noticed with FMT Maven plugin. The formatter does not handle CR LF line endings rather it converts everything to LF which will mess with commits because it causes high rewrites. So you would need to ensure that git attributes is in all repos as well to avoid incorrect commit history.
-- Tomas Bjerre is using it on his repos: https://github.com/jenkinsci/violation-comments-to-stash-plugin See this commit message: https://github.com/jenkinsci/violation-comments-to-stash-plugin/pull/50/commits/1f2fd46632aaf65493f41bf74e660710221fd70f tirsdag den 24. juli 2018 kl. 03.36.42 UTC+2 skrev Mark Waite:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/c3ef8d20-8332-47f6-9803-68178421aac0%40googlegroups.com. For more options, visit https://groups.google.com/d/optout. |
I am more of the philosophy of "teach a man to code and he will eat for a lifetime" approach. So most of my "originally authored" plugins have checkstyle rules applied to the build. It is by no means perfect, but I haven't had to complain about white spaces in PRs to those plugins in a long time :) /B Den tis 24 juli 2018 kl 09:48 skrev Joseph P <[hidden email]>:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CALzHZS2e5mS4EN4AzCRVhVa0-nsT2B7t47QeMDMBTFPGgwyjaw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Joseph P
On Tue, Jul 24, 2018 at 1:48 AM Joseph P <[hidden email]> wrote:
I like the idea of including a .gitattributes file as part of the transition to optional automatic formatting in a repository. I'll include that in the proposal. Additional files were needed with the transition to support incrementals and that transition seems to be progressing very well. I prefer (and accept) the intentional setting of line endings. I consider that part of the standardization of the formatting. I think that maintainers that don't want line endings standardized or that cannot accept LF as the standard would choose to not enable automatic formatting in the plugins they maintain. Thanks, Mark Waite
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtGoTBz3oYV5mROLmQDuxTo%3D7B8Z%3D-iurUBboC%3Dj7W8jHg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Mark Waite-2
On Mon, Jul 23, 2018 at 9:36 PM Mark Waite <[hidden email]> wrote:
> Automatic - when source code formatting is enabled, it happens automatically as part of the maven compile phase Meaning that `mvn compile` modifies `src/main/java/**/*.java`? Convenient if potentially surprising. But what about pull requests in which the author never ran Maven to this goal, and edited sources do not match the format—is there any way for the PR build to fail in this case? (Note that this does _not_ mean the PR author never tested/tried the code. Some IDEs run internal compilation, bypassing Maven.) > How would a prototype of optional automated source formatting be evaluated? By releasing a beta version of the plugin parent pom? Just by doing a regular release of the plugin parent POM. If the change is a dud, we just back that configuration out in a later release. > Is "google java format" an acceptable code format for those plugin authors that are willing to adopt automated code formatting? BTW I am leery of automated source formatting generally, but if I have to comply with a format, it is nice that it allows inline determinative annotations such as `@Override` or `@Test` without someone yelling at me about some arbitrary Sun Microsystems-era guideline. :-) It remains unfortunate that it prohibits inline annotations with any parameters even when they are determiners, like the second line in @Parameterized.Parameter public String x; @Parameterized.Parameters(name = "x={0}") public static String[] allowedXs() { // … } since inserting a line break would make `Parameters` look like a mere attribute, which it is not. My heart was also warmed by this note: > Never make your code less readable simply out of fear that some programs might not handle non-ASCII characters properly. If that should happen, those programs are broken and they must be fixed. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1udMOYvASdbT5LOn_Se8bOtmzCxx9Y53T6zG32fCpuxA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Robert Sandell-2
On Tue, Jul 24, 2018 at 4:31 AM Robert Sandell <[hidden email]> wrote:
I like that as an alternative for consideration. My experience with checkstyle has not been as positive as yours. I'll try it with platformlabeler plugin to see if I can find settings that work for me. At a minimum, I'll include in an eventual JEP a suggestion that checkstyle is an alternative to source code formatting, since it will check the formatting without modifying the formatting. Mark Waite
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtHUtX84zkSDR3fHskBxr6-XWVNuHT%2B%2BjKE-xLmEd4rqCA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Mark Waite-2
Great idea and initiative IMO, thanks a bunch for driving this Mark. Over the years, I've become more and more strongly opinionated that *a* common format is defined, and easily applied, and less and less about the precise format. IOW, I generally do have opinions about where curly brackets and others should go, but I'll happily put them aside if this helps us define *something*. @Mark you can use https://github.com/jenkinsci/buildtriggerbadge-plugin or https://github.com/jenkinsci/essentials-plugin and few others to test drive this. https://github.com/jenkinsci/radiatorview-plugin might also be interesting as it has a bit more code, and has been existing for quite long I think. -- Baptiste Le mar. 24 juil. 2018 à 03:36, Mark Waite <[hidden email]> a écrit :
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS4oBgvoAZnWDh8Fw9HfNATZFcrg3ZDKm4m0LwLYWti7Ew%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
The auto format should occur before the compile stage, probably during process-sources.
On Tue, Jul 24, 2018, 6:00 AM Baptiste Mathus <[hidden email]> wrote:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAA0qCNwGXcwroH95QyCORW4cYXx8h0BsUiLO4NwAZtF2Vh2Siw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Jesse Glick-4
On Tue, Jul 24, 2018 at 6:30 AM Jesse Glick <[hidden email]> wrote: On Mon, Jul 23, 2018 at 9:36 PM Mark Waite <[hidden email]> wrote: That was my intent. Based on the later comment from Liam Newman, I'd change the proposal to optionally insert the fmt-mvn-plugin into the process-sources phase (before compile). But what about pull requests in which the author never ran Maven to That's an excellent point. Do you think it would be acceptable to fail the process-sources phase (or the compile phase) if it detected that we were running inside Jenkins and a file had been modified at the end of the process-sources phase? Is there a reasonable way to do that in maven? The fmt-mvn-plugin has a "check" goal which will report files which would be modified without modifying them. I assume that could be used in an optional step in the Jenkinsfile to decide if a pull request was not formatted to match the settings of that plugin. > How would a prototype of optional automated source formatting be evaluated? By releasing a beta version of the plugin parent pom? I'm not sure how to address this specific concern. Any suggestions? Mark Waite My heart was also warmed by this note: -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtHz1wzRi8csz_fnCqDk80BB%2BXXrTxHzj0%2BedSt6J6bVZw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
On Tue, Jul 24, 2018 at 6:21 PM Mark Waite <[hidden email]> wrote:
> Do you think it would be acceptable to fail the process-sources phase (or the compile phase) if it detected that we were running inside Jenkins and a file had been modified at the end of the process-sources phase? Would work. We actually do something similar in core: https://github.com/jenkinsci/jenkins/blob/1a11fa39b567fd7ff64e61fab23aa18a123f2d97/Jenkinsfile#L48 > The fmt-mvn-plugin has a "check" goal which will report files which would be modified without modifying them. …and fails if so: https://github.com/coveo/fmt-maven-plugin/blob/145bbfbc0e9eb6f2aaf8b6bdf930efef5a8ddbef/src/main/java/com/coveo/Check.java#L63 > I assume that could be used in an optional step in the Jenkinsfile You could put this into a Maven profile and activate it from `buildPlugin.groovy`. > I'm not sure how to address this specific concern. Not a concern; you may disregard. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1W8ZYb6_R67b55pcdke-CZHwKkkfr%2BnexW569YfkhJSA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
Might also need to turn it off during "mvn release". It would be very strange, and probably bad, if the -sources.jar didn't match the sources on GitHub for the release tag. Or maybe that just gets pushed together with the "[maven-release-plugin] prepare release ..." commit? /B Den ons 25 juli 2018 kl 00:52 skrev Jesse Glick <[hidden email]>: On Tue, Jul 24, 2018 at 6:21 PM Mark Waite <[hidden email]> wrote: You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CALzHZS3aFtSwivp3SqrFXLRo0tb4WBFj0CoasHXOhBRJw1FYbw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Mark Waite-2
Just to touch on the line endings issue... See https://github.com/jenkinsci/vstestrunner-plugin/pull/16 when it becomes an issue. I just wanted to fix a minor thing but because of the maintainer did a commit where they messed with line endings it screwed up the history for https://github.com/jenkinsci/vstestrunner-plugin/pull/17
-- tirsdag den 24. juli 2018 kl. 14.30.01 UTC+2 skrev Mark Waite:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/a78c887f-e3ed-4d8a-a750-9ea71865c9a5%40googlegroups.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Robert Sandell-2
Or maybe that just gets pushed together with the "[maven-release-plugin] prepare release ..." commit? onsdag den 25. juli 2018 kl. 11.14.00 UTC+2 skrev Robert Sandell:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/a927f155-298d-42c3-9576-61f9e8b9215d%40googlegroups.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Joseph P
On Wed, Jul 25, 2018 at 4:44 AM Joseph P <[hidden email]> wrote:
I see the changes in line endings between those two commits but am not clear on the actions which are needed by a maintainer to avoid the problem. Is it enough to have the .gitattributes file that sets the line endings of files that will be automatically formatted? If that's not enough, could you explain further what is needed? Mark Waite
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtH1BHjd-ji2XzSY00Np3BJ-eENPbACQz9JEJJyOeQSqAQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
The action needed to avoid line endings being messed with is
However this would be enough for the formatter
And if the maintainer needs something more fancy they can customise it further but lucky shouldn't have anything like Visual Studio
If a maintainer mess it up the answer would be to add the .gitattributes and run
Like I did in the first PR. onsdag den 25. juli 2018 kl. 14.06.31 UTC+2 skrev Mark Waite:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/cb84f1a3-fcc6-4685-9e12-c60899654680%40googlegroups.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Mark Waite-2
This is great stuff! Thanks Mark =)
-- I've been doing this in my plugins for a couple of years now and I have only nice things to say. It helps contributors a lot as well as they can focus on the implementation and not fiddle with formatting. Den tisdag 24 juli 2018 kl. 03:36:42 UTC+2 skrev Mark Waite:
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/983cb8b3-e442-42a5-a6b5-85cd5b5711ae%40googlegroups.com. For more options, visit https://groups.google.com/d/optout. |
In reply to this post by Robert Sandell-2
On Wed, Jul 25, 2018 at 5:14 AM Robert Sandell <[hidden email]> wrote:
> Might also need to turn it off during "mvn release". And Incrementals. This is feeling increasingly scary. Generally speaking, I would find it an unwelcome surprise if a `mvn install` or whatever _modified_ my source tree. It is OK for some plugin to cause `validate` or `prepare-sources` or some such phase to fail when sources are not in compliance with some standard (so long as this check is fast enough to not burden a developer workflow, or is suppressed during some sort of quick build profile TBD)—but at most such a mojo should print a failure message to the effect of > Source formatting out of compliance. To fix, run: mvn fmt:format so that the change to sources is a conscious decision, one I know I need to commit (perhaps `--amend`) and include in my PR. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr18En5T-JF%2BOk4tc_%3D-CTp1BNQVA%3DHT6x5jvddw5vk8_w%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
On Wed, Jul 25, 2018 at 7:30 AM Jesse Glick <[hidden email]> wrote: On Wed, Jul 25, 2018 at 5:14 AM Robert Sandell <[hidden email]> wrote: That's a good insight. I prefer that if the maintainer has chosen to enable automatic source formatting that it is "always on". I think your observation means that you would probably not choose to enable automatic source formatting in the plugins you maintain if the formatting is "always on". Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase? If that were the path we followed, then I would assume that the new argument added to buildPlugin for ci.jenkins.io would perform a fmt-maven-plugin:check if formatting were enabled for that plugin. Mark Waite -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtEbnNZh518KFbjm3Zy0%3D7QAa%2BoWJPDtnHh5uLgTgCmEyw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
On Wed, Jul 25, 2018 at 9:40 AM Mark Waite <[hidden email]> wrote:
> I think your observation means that you would probably not choose to enable automatic source formatting in the plugins you maintain if the formatting is "always on". No, I am referring to _contributing_ to such a plugin maintained by someone else. I would not expect a `mvn` command in the default lifecycle to ever modify a versioned file. That generally counts as a serious bug in the build setup. > Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase? There are already two modes of operation being discussed. The question is what happens by default when I clone somebody’s repo, edit a couple of lines, run tests from my IDE, commit, maybe do some follow-up Maven build, and file a PR against it. Am I suddenly left with uncommitted modifications in my source tree? Worse, what happens when I edit some lines, instruct my IDE to run a Maven build to run tests (saving all modified files in the process), then start making some follow-up edits to sources while I am waiting for Maven to finish because I realize I forgot something? I am going to get a conflict and a confusing dialog in my IDE asking me to decide whether to save my actual edits, or discard and reload from disk. Perhaps the reformatting during a default lifecycle phase could be something that you (the developer working locally, not the plugin maintainer!) could explicitly opt in to, for example with a local profile in `~/.m2/settings.xml`. Then you are at least expecting this to happen and are making corresponding accommodations in your development workflow. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr0JHcOprUBit1EH-xsYbgtRJbsk_W5kuWy%2BEphVnLPzPw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
On Wed, Jul 25, 2018 at 9:21 AM Jesse Glick <[hidden email]> wrote: On Wed, Jul 25, 2018 at 9:40 AM Mark Waite <[hidden email]> wrote: Thanks for the clarification. I hadn't seen that as a serious bug in the build setup. That changes how I was intending to approach the problem. I think that means that if optional source formatting is enabled, then the process-sources phase would execute fmt-maven-plugin:check and fail the build if formatting changes were needed. The developer would then run `mvn fmt-maven-plugin:format` to format the files, then return to running their usual phase (`mvn install` or `mvn verify` or ...). > Would it meet your needs if there were two modes of operation, one that includes a fmt-maven-plugin:check in the process-sources phase without modifying source code, and one that performs the formatting in the process-sources phase? I like that idea. The plugin maintainer chooses to have source formatting checked as part of a default phase. The plugin developer may either perform the required source formatting as a separate maven command or may choose to configure their local development environment with a local profile in ~/.m2/settings.xml. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAO49JtGEr-dTVAnDdh%3Dc9pO2hdtZPRo%3DHmVbuZEM73UenfQ6Kw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout. |
Free forum by Nabble | Edit this page |