Split and Organize Checkstyle inputs by Test for NoWhitespaceAfter #3931

Closed
romani opened this Issue Mar 5, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@romani
Member

romani commented Mar 5, 2017

subtask of #3891 .

do a copy of all test input files to separate folder for NoWhitespaceAfter .
Not any other Check should use new inputs.

@romani

This comment has been minimized.

Show comment
Hide comment
@bansalayush

This comment has been minimized.

Show comment
Hide comment

I am on it

@bansalayush

This comment has been minimized.

Show comment
Hide comment
@bansalayush

bansalayush Mar 7, 2017

Kindly review the modifications I made and let me know if it'd be alright to make a pull request for it.

Kindly review the modifications I made and let me know if it'd be alright to make a pull request for it.

@sabaka

This comment has been minimized.

Show comment
Hide comment
@sabaka

sabaka Mar 7, 2017

Contributor

@bansalayush , where we can find changes you made?
I've tried to find any commits in your fork, but there is only copy of the main repo.

Contributor

sabaka commented Mar 7, 2017

@bansalayush , where we can find changes you made?
I've tried to find any commits in your fork, but there is only copy of the main repo.

@bansalayush

This comment has been minimized.

Show comment
Hide comment
@bansalayush

bansalayush Mar 7, 2017

@sabaka my bad . Now you can see the changes in my fork https://github.com/bansalayush/checkstyle

@sabaka my bad . Now you can see the changes in my fork https://github.com/bansalayush/checkstyle

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 7, 2017

Member

@bansalayush You must start a PR before we start reviewing your changes. We only review once CI in PR passes. CI will catch most problems. This also keeps the issue clean of any implementation problems and only focuses and the issue.

Member

rnveach commented Mar 7, 2017

@bansalayush You must start a PR before we start reviewing your changes. We only review once CI in PR passes. CI will catch most problems. This also keeps the issue clean of any implementation problems and only focuses and the issue.

@bansalayush

This comment has been minimized.

Show comment
Hide comment
@bansalayush

bansalayush Mar 8, 2017

@rnveach Umm... What is PR and CI???

bansalayush commented Mar 8, 2017

@rnveach Umm... What is PR and CI???

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 8, 2017

Member

@bansalayush I assume you are familar with github and git? Please let us know.
https://help.github.com/articles/github-glossary/
https://en.wikipedia.org/wiki/Continuous_integration
PR = pull request, CI = continuous integration

Member

rnveach commented Mar 8, 2017

@bansalayush I assume you are familar with github and git? Please let us know.
https://help.github.com/articles/github-glossary/
https://en.wikipedia.org/wiki/Continuous_integration
PR = pull request, CI = continuous integration

@bansalayush

This comment has been minimized.

Show comment
Hide comment
@bansalayush

bansalayush Mar 9, 2017

Yeah, I have been using git for quiet some time but never collaborated with anyone.So I lag in collaboration area and will need help. I have committed changes on my fork and pushed them but my fork is not up to date . I'm stuck here. I need to update my fork without deleting my pushed changes . I suppose only after this i can make a pull request . Sorry for the trouble .

bansalayush commented Mar 9, 2017

Yeah, I have been using git for quiet some time but never collaborated with anyone.So I lag in collaboration area and will need help. I have committed changes on my fork and pushed them but my fork is not up to date . I'm stuck here. I need to update my fork without deleting my pushed changes . I suppose only after this i can make a pull request . Sorry for the trouble .

@sabaka

This comment has been minimized.

Show comment
Hide comment
@sabaka

sabaka Mar 9, 2017

Contributor

@bansalayush try to make all changes in your fork in separate branches (one per issue) and keep master branch up to date with main checkstyle repo (I use two remotes: origin for my fork and upstream for main repo). With this approach, when your fix is ready, you can do pull from upstream to your master, do rebase of your topic branch and send PR from branch.

Now I would suggest you to do following:

  1. switch to branch from master
  2. push branch to your fork
  3. reset your commit in master
  4. pull master branch from main repo
  5. rebase you branch
  6. send PR from branch
Contributor

sabaka commented Mar 9, 2017

@bansalayush try to make all changes in your fork in separate branches (one per issue) and keep master branch up to date with main checkstyle repo (I use two remotes: origin for my fork and upstream for main repo). With this approach, when your fix is ready, you can do pull from upstream to your master, do rebase of your topic branch and send PR from branch.

Now I would suggest you to do following:

  1. switch to branch from master
  2. push branch to your fork
  3. reset your commit in master
  4. pull master branch from main repo
  5. rebase you branch
  6. send PR from branch
@bansalayush

This comment has been minimized.

Show comment
Hide comment
@bansalayush

bansalayush Mar 10, 2017

I followed your instructions.
But during CI test i couldn't pass any of the test. What could i have done wrong . Appreciate your help and patience.

I followed your instructions.
But during CI test i couldn't pass any of the test. What could i have done wrong . Appreciate your help and patience.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 10, 2017

Member

please make sure that you can pass tests before your changes on your local, if it pass it mean that your changes caused problems - investigate what change cause first problem.

Member

romani commented Mar 10, 2017

please make sure that you can pass tests before your changes on your local, if it pass it mean that your changes caused problems - investigate what change cause first problem.

@Kietzmann

This comment has been minimized.

Show comment
Hide comment
@Kietzmann

Kietzmann Apr 10, 2017

Contributor

Hi,
May I deal with this issue?

Contributor

Kietzmann commented Apr 10, 2017

Hi,
May I deal with this issue?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 10, 2017

Member

@Kietzmann , yes you can

Member

romani commented Apr 10, 2017

@Kietzmann , yes you can

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 10, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 11, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 12, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 13, 2017

Kietzmann added a commit to Kietzmann/checkstyle that referenced this issue Apr 14, 2017

romani added a commit that referenced this issue Apr 17, 2017

@romani romani added this to the 7.7 milestone Apr 17, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 17, 2017

Member

fix is merged

Member

romani commented Apr 17, 2017

fix is merged

@romani romani closed this Apr 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment