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 #4425: handle multi-dimensional array part of type in CheckUtils.createFullType() #4460

Closed
wants to merge 1 commit into from

Conversation

kevin-wayne
Copy link

@kevin-wayne kevin-wayne commented Jun 15, 2017

…e #4425

createFullType() previously ignored the array part of the type in one-dimensional arrays (but not multi-dimensional arrays). This led to an upstream bug (#4425) in IllegalTypeCheck.

@codecov-io
Copy link

codecov-io commented Jun 15, 2017

Codecov Report

Merging #4460 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4460   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         284     284           
  Lines       15371   15368    -3     
  Branches     3500    3500           
======================================
- Hits        15371   15368    -3
Impacted Files Coverage Δ
.../puppycrawl/tools/checkstyle/utils/CheckUtils.java 100% <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 d6ea9fd...eb19371. Read the comment docs.

@kevin-wayne kevin-wayne reopened this Jun 15, 2017
@rnveach
Copy link
Member

rnveach commented Jun 15, 2017

@kevin-wayne Please read over https://github.com/checkstyle/checkstyle/wiki/PR-rules .

You don't need to delete fork branch, close/open PR to apply new changes.
Just amend the commit and force push your changes and github will pick them up automatically.

Please keep number of commits as 1.
Once CI starts passing, we will start reviewing.

@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 Jun 29, 2017

@kevin-wayne, are you going to continue ? if not , please let us know , I will close PR and let comment in issue that anybody is welcome to continue.

@kevin-wayne
Copy link
Author

Sorry @romani. Couldn't quite figure out how to amend and force push in git. (Still a novice with git and was just using the web interface, where that doesn't seem to be an option.)

@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

@kevin-wayne , do still have problems with environment ? can we help/guide somehow ?

@kevin-wayne
Copy link
Author

@romani Thanks for guidance and patience.

I followed the instructions in beginning_development.html and everything seems clear from command line, including rebase and mvn. (Sorry for not noticing these instructions when I started.) In the future, I'll know how to force push a branch I create from the command line. Unfortunately, the branch I created from the command line (my-new-check) is different from the ones I created in the github web interface (patch-1 and patch-2). Not wanting to create a third PR, let me know how to proceed. Thanks!

@romani
Copy link
Member

romani commented Jul 4, 2017

@kevin-wayne , this PR is based on "kevin-wayne:patch-2", so patch-2 branch is used , you can keep this PR and your local my-new-check branch , but you need to push your local my-new-check to patch-2 on remote - git push origin patch-2 --force.
but it is better to avoid complications and rename you local to patch-2 and push it to the same branch on remote.

@romani
Copy link
Member

romani commented Jul 5, 2017

@kevin-wayne , please squash all in one commit - https://github.com/checkstyle/checkstyle/wiki/PR-rules

@rnveach
Copy link
Member

rnveach commented Jul 6, 2017

@kevin-wayne As you make changes, you need to add tests to prove your changes work and are correct.
You are making changes for IllegalType but I don't see any new tests or changes in existing ones.
Please add a test to IllegalType to showcase change.

@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

@kevin-wayne , if you do not have time to finish this PR, please let us know we have active contributors that could continue your work.

@kevin-wayne
Copy link
Author

Sorry, please have another contributor finish the PR. I do appreciate all of your help and patience.

@romani
Copy link
Member

romani commented Aug 31, 2017

I marked issue as abandoned, any contributor is OK to continue this work.

@romani
Copy link
Member

romani commented Sep 8, 2017

I am closing this in favor of #5073

@romani romani closed this Sep 8, 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.

None yet

5 participants