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

6635: Add whitespace rule to JMC jcheck configuration #4

Closed
wants to merge 1 commit into from

Conversation

@RealCLanger
Copy link
Contributor

RealCLanger commented Nov 15, 2019

This is my first PR with skara :)

I think it would be nice if jcheck would also check for unnecessary whitespace.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JMC-6635: Add whitespace rule to JMC jcheck configuration

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 2019

👋 Welcome back clanger! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Nov 15, 2019
@mlbridge
Copy link

mlbridge bot commented Nov 15, 2019

Webrevs

@thegreystone
Copy link
Collaborator

thegreystone commented Nov 15, 2019

When I discussed this with Erik (Helin), he told me there could be a problem with us using tabs instead of spaces. I think we should instead try to make spotless part of the process.

@thegreystone
Copy link
Collaborator

thegreystone commented Nov 15, 2019

I added a PR for a first trial run with spotless here: #5, let me know what you think!

@RealCLanger
Copy link
Contributor Author

RealCLanger commented Nov 18, 2019

When I discussed this with Erik (Helin), he told me there could be a problem with us using tabs instead of spaces. I think we should instead try to make spotless part of the process.

What could be the problems? I think we would not want to have tabs in java code, would we?

@RealCLanger
Copy link
Contributor Author

RealCLanger commented Nov 18, 2019

I added a PR for a first trial run with spotless here: #5, let me know what you think!

Cool. I'll look into it.

@thegreystone
Copy link
Collaborator

thegreystone commented Nov 18, 2019

When I discussed this with Erik (Helin), he told me there could be a problem with us using tabs instead of spaces. I think we should instead try to make spotless part of the process.

What could be the problems? I think we would not want to have tabs in java code, would we?

JMC is formatting code with tabs. One indentation level is one tab.

So, if you want to read your code with 2 spaces for one indentation step:
https://github.com/openjdk/jmc/blob/master/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCMethod.java?ts=2

And if you want to read your code with 6 spaces for one indentation step:
https://github.com/openjdk/jmc/blob/master/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/IMCMethod.java?ts=6

So, basically set your IDE to however you like the tabs visualized. One size doesn't fit all, and does not have to. :)

@RealCLanger
Copy link
Contributor Author

RealCLanger commented Nov 22, 2019

Ok, I guess I withdraw this PR then. I wasn't aware of the usage of tabs in JMC :)

@RealCLanger RealCLanger deleted the RealCLanger:jcheck-whitespace branch Nov 22, 2019
thegreystone added a commit to thegreystone/jmc that referenced this pull request Nov 23, 2019
* JMC-5458: Support Class-Redefinitions

Summary: Handling existing class name collisions and supporting reverting instrumentation of a method
Contributed-by: Jessye Coleman-Shapiro <jescolem@redhat.com>

* Removed unneccessary logic

* Removed JDK 11+ functionality requested from thegreystone/jmc#4

* Make general function to specify what the transfromed state should be
aptmac added a commit to aptmac/jmc that referenced this pull request Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.