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

Fix spotlessFiles parameter tests #737

Merged
merged 2 commits into from
Nov 17, 2020
Merged

Conversation

driv
Copy link
Contributor

@driv driv commented Nov 15, 2020

  • (,| and ) should not be escaped with \, otherwise they get
    interpreted as literals and no file matches
  • On Windows paths use backslashes, so \\\\ has to be used.
    2 for java strings and 2 since this parameter is treated as a regex.

Few remarks:

  • No issue for this, I just couldn't make it run check on Windows. Changes only on Tests.
  • I don't have a unix machine right now to test it on.
  • I'm not sure if/how it was even running for unix. .*/src/main/java/test\\(1\\|3\\).java should have been interpreted as literal.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Hmmm... the unix tests were passing, but now they are failing. I see your point about the parens being interpreted as literal, I'm not sure why this worked before, but it did.

}

private boolean isOnWindows() {
return System.getProperty("os.name").startsWith("Windows");
Copy link
Member

Choose a reason for hiding this comment

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

I would use

/** Returns true if this JVM is running on a windows machine. */
public static boolean machineIsWin() {
return machineIsWin;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was 2 seconds away from pushing this change. :)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, guess my review was sloppy. "Is the project better now?" If the answer is yes, then I try to get people's stuff merged-in ASAP - I try to err on the side of not blocking people once I'm confident that it's not making new bugs or vulnerabilities. It's not a big deal to have this duplicated. If you want to swap this out in some future PR then fine, if it stays duplicated that's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll look into the this issue #710 and add it there.

@nedtwigg
Copy link
Member

Also, needs a changelog entry in plugin-maven/CHANGES.md. Can use this PR as the link.

@driv
Copy link
Contributor Author

driv commented Nov 16, 2020

Any idea what's going on with the npm one? Does it use a different java version?

@nedtwigg
Copy link
Member

Spotless is JRE 8+

test_npm_8:
<< : *env_gradle
environment:
# java doesn't play nice with containers, it tries to hog the entire machine
# https://circleci.com/blog/how-to-handle-java-oom-errors/
# try the experimental JVM option
_JAVA_OPTIONS: "-XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap"
docker:
- image: cimg/openjdk:8.0-node
steps:
- checkout
- *restore_cache_wrapper
- *restore_cache_deps
- run:
name: gradlew npmTest
command: ./gradlew npmTest --build-cache

The Path method you are using is present in JRE 11, but not in JRE 8.

- Unix regex for wrapped in `'`. `\\` is not needed anymore.
- On Windows paths use backslashes, so `\\\\` has to be used.
2 for java strings and 2 since this parameter is treated as a regex.
@nedtwigg nedtwigg merged commit d9474fb into diffplug:main Nov 17, 2020
@nedtwigg
Copy link
Member

Thanks for the fix!

@driv
Copy link
Contributor Author

driv commented Nov 18, 2020

Thanks for the fix!

Thanks for your support! You make it really easy for people to contribute.

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