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

Added Eclipse 4.20 formatter support. #940

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Added Eclipse 4.20 formatter support. #940

merged 1 commit into from
Sep 20, 2021

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Sep 19, 2021

Unlike for previous Eclipse version upgrades I would prefer a feedback and not pushing directly into main.

  1. I used only Changed keyword in the change log, since I saw that this has been done for other steps. Remind me, where do I found documentation how Changed affects the version. Expected documentation here.
  2. I reverted the Eclipse test approach introduced with Migrate tests to JUnit 5 #900, where only versions are tested on higher JVMs which have not been tested by lower JVMs. This would force me to switch JVM all the time. See JDT example.
  3. I introduced work-around for non-sermantic Eclipse versions

Outlook:
Since quite some time it vexes me that we test so many Eclipse versions. I propose to test exemplary once which make a difference in the code:

  • Min/Max supported by a JVM
  • Versions where interface has change (covered by Min/Max)
  • Non-Semantic versions (also covered by Min/Max)

The only thing we loose is a complete validation all lock files. But we don't change the lock files for older versions. Hence we only check whether versions specified in the lock files are still available on MavenCentral. So in my opinion we just waste a lot of energy downloading all the different historical version of Eclipse. Will provide proposal to change Eclipse testing in different PR.

@fvgh
Copy link
Member Author

fvgh commented Sep 19, 2021

CI has severe problems with dependency lookup from local cache. Example:

Could not get resource 'http://localhost:8080/repository/internal/com/diffplug/spotless/spotless-eclipse-cdt/10.0.0/spotless-eclipse-cdt-10.0.0.pom'

Problem in CI is reproducible. CI job restart delivers similar result.
Can't find source of problem. Artifacts are all available on MavenCentral. Cannot reproduce problem locally.

@nedtwigg I am afraid I need your help on this one.

@fvgh
Copy link
Member Author

fvgh commented Sep 20, 2021

@nedtwigg I am afraid I need your help on this one.
Thanks @nedtwigg. Shouldn't have bothered you, but look at it again next morning. Was just too tired.

@nedtwigg
Copy link
Member

I used only Changed keyword in the change log, since I saw that this has been done for other steps. Remind me, where do I found documentation how Changed affects the version.

Defining semver as breaking.added.fixed

  • **BREAKING** bumps breaking
  • ### Added bumps added
  • anything else bumps fixed

IMO, the point of semver is to communicate integration risk to the end-user. As a user, if you adopt:

  • breaking then you should expect that integration work is required
  • added then you might want to integrate some new capabilities, but you don't have to, but you should be prepared for accidental regressions caused by an ambitious new feature
  • fixed then you should have high confidence that whatever you were doing before will still work without any effort from you, just faster and better

I think adding support for a new version which required structural changes counts as added level of risk, whereas a routine addition with very little chance of regression counts more as fixed level of risk. E.g. when a new version of a formatter has changed maven coordinate, and we have to parse out the version to determine which maven coordinate to use, that seems like a "fix" to me. But if a formatter changes its entry-point, such that supporting new versions of that formatter required a rewrite of the integration code, that seems like an added to me.

There is a strong case to make that anytime we bump our default version, that's at least an added, if not a breaking, because it might change the way that files are formatted. But I think there's a stronger case that a user who is using the default versions has said "I want the latest style", so adjusting to the latest style is always a low-risk fix for them, rather than a medium-risk new feature. For users who have pinned their formatter version, a change in defaults has no effect and is especially low-risk.

So that is why I put version bumps as ### Changed, but mostly I don't care and merge them however people have written them. The real point of spotless and spotless-changelog is to help this minutiae slip into the background a bit.

I reverted the Eclipse test approach...

Looks good.

introduced work-around for non-sermantic Eclipse versions

Looks good.

Will provide proposal to change Eclipse testing in different PR.

Looks good.

To my eye this PR is ready to merge, look good to you too?

@nedtwigg
Copy link
Member

Shouldn't have bothered you

Nonsense! That's what second pairs of eyes are for :)

@fvgh fvgh merged commit b0b4e78 into main Sep 20, 2021
@fvgh fvgh deleted the eclipse-4.20 branch September 20, 2021 18:39
@fvgh
Copy link
Member Author

fvgh commented Sep 20, 2021

@nedtwigg Sorry, you lost me with 8786261. Your fix is understood. I also understand your comment about the missing build on merge tip. But I do not understand why the problem did not occurre building 44ad6d9, where the JUnit version was switched from 4 to 5. What did I miss?

@nedtwigg
Copy link
Member

Here is a timeline:

  • Configuration cache PR begins, based on JUnit 4
  • JUnit 4->5 PR begins, based on JUnit 4
  • JUnit 4->5 PR is merged, JUnit 4 is now unavailable
  • Configuration cache PR is still based on JUnit 4. Because it didn't touch any build.gradle or any existing tests, there is no merge conflict to resolve. But once merged, the newly introduced configuration cache tests fail, because they are counting on JUnit 4 being around, but it is gone.

Some CI systems, like Travis, test both the tip of the PR, and also a hypothetical merge commit. Any time the main branch moves, every PR needs to rerun CI to test hypothetical merge. The last time I checked, CircleCI did not support that and their staff was arguing on their forum that it was an unnecessary feature. This is an example of why it's important (every PR should have its full test suite executed twice, once on the tip, and if that succeeds, again on a hypothetical merge, which must be updated anytime the hypothetical merge changes).

@fvgh
Copy link
Member Author

fvgh commented Sep 20, 2021

@nedtwigg OK, what I missed as the fact that you mentioned #720 in 8786261 😄 I was too focused on #940. Thanks for clarification.

@nedtwigg
Copy link
Member

Released in plugin-gradle 5.15.1 and plugin-maven 2.13.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants