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

Issue #4410: Do not allow whitespace after @ sign #4412

Closed
wants to merge 2 commits into from
Closed

Issue #4410: Do not allow whitespace after @ sign #4412

wants to merge 2 commits into from

Conversation

strkkk
Copy link
Member

@strkkk strkkk commented Jun 3, 2017

Issue #4410

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

Please generate diff report for your changes to make sure we don't see any issues.
https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation

@@ -286,4 +286,11 @@ public void parentheses() {

public static void testNoWhitespaceBeforeEllipses(String ... args) {
}

@ interface WithWhitespace {}
Copy link
Member

Choose a reason for hiding this comment

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

Please add some good cases where no violation is produced.
Also add one with line breaks after instead of space.

@@ -86,6 +87,7 @@
public int[] getDefaultTokens() {
return new int[] {
TokenTypes.ARRAY_INIT,
TokenTypes.AT,
Copy link
Member

Choose a reason for hiding this comment

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

@romani Please confirm it is ok to be default.
I don't see anything wrong as most people don't write with space after.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, ok to be default.

@romani
Copy link
Member

romani commented Jun 4, 2017

@strkkk , to resolve CIs, please rebase.
CIs have to be green to do a merge.

@codecov-io
Copy link

Codecov Report

Merging #4412 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4412   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         285     285           
  Lines       15292   15292           
  Branches     3477    3477           
======================================
  Hits        15292   15292
Impacted Files Coverage Δ
...tyle/checks/whitespace/NoWhitespaceAfterCheck.java 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28139b2...b9931a9. Read the comment docs.

@rnveach
Copy link
Member

rnveach commented Jun 6, 2017

@strkkk number of commits must remain as one. You must also rebase on latest master.
Please reply to my points as Done so I know when to review the new changes.

@strkkk
Copy link
Member Author

strkkk commented Jun 7, 2017

@rnveach
Ok, I will do it
Also, how can I see why wercker build failed? It tells me that I am not authorized even if I am logged in.

Rebase - done.
Changes are in a new commit - will rework it.
I had some troubles with generating diff report, it is in process.

@rnveach
Copy link
Member

rnveach commented Jun 7, 2017

Also, how can I see why wercker build failed?

It is an issue with wercker. They don't allow anonymous viewing.
It fails because of an old problem with master, which is why we are telling you to rebase.

@romani
Copy link
Member

romani commented Jun 7, 2017

@strkkk ,

as already mentioned:

  1. please squash all commits in one
  2. rebase on latest our master
  3. provide regression testing report on all projects in property file (I doubt that you can find a lot violations, as we should use as much possible real code projects) - https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation , share report with us https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#deploy-report

@rnveach
Copy link
Member

rnveach commented Jun 14, 2017

@strkkk ping

@strkkk
Copy link
Member Author

strkkk commented Jun 15, 2017

@rnveach yes, I am here. I think I will finish it on weekend.

@romani
Copy link
Member

romani commented Jun 22, 2017

Please also rebase on latest master, we did travis fix, it should pass.

@romani
Copy link
Member

romani commented Jul 2, 2017

checkstyle 8.0 was released, please rebase on latest master.
after each release it is mandatory to rebase to avoid CIs failures.

@romani
Copy link
Member

romani commented Jul 3, 2017

@strkkk , are you still plan to finish this PR ?
if you do not have time please let us know.

@romani
Copy link
Member

romani commented Jul 9, 2017

@strkkk , to resolve wercker problem please rebase on our latest master

@romani
Copy link
Member

romani commented Aug 31, 2017

we released 8.2 version, please rebase all your PRs to our latest master to avoid CI problems.

This PR is abandoned .... any contributor is ok to continue

@romani
Copy link
Member

romani commented Aug 31, 2017

looks like we lost connection to contributor.
I marked issue as abandoned, any contributor is OK to continue this work.

@romani
Copy link
Member

romani commented Sep 13, 2017

work will be continued at #5108

@romani romani closed this Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants