checkstyle

checkstyle copied to clipboard

fix for #9280

In JavadocVariableCheck, replace Scope scope and Scope excludeScope by AccessModifierOption[] accessModifiers.

Whether the JavadocVariableCheck is applied to a given variable still depends on the variable's modifier and on the variable's surrounding scope, like it was before. I.e. to run the check on a public field in a private class, accessModifiers must include private, but not necessarily public. If it was intended to change this behaviour, let me know.

@martin-mfg thanks for your interest in checkstyle.

I think you misunderstood issue a little bit.

You should not remove existing properties and replace them with new one. It is a breaking change and issue didn't mean it. Only type of fields should be changed to AccessModifierOption

Hi, I intended to make changes similar to those in f91b1af, because of this comment: https://github.com/checkstyle/checkstyle/issues/3183#issuecomment-283539970.

If I understand you correctly, JavadocVariableCheck should instead have such fields:

private AccessModifierOption scope;
private AccessModifierOption excludeScope;

However, I don't see yet how that solves the original issue. anoninner, which is part of the original issue example, doesn't exist in AccessModifierOption, so the original example wouldn't work anymore. But if I am not mistaken, the broader problem was that according to the Scope semantics, public is a subscope of protected is a subscope of package is a subscope of private.

see https://github.com/checkstyle/checkstyle/blob/b51e96403cf844fa805113e4570f33bc36314388/src/main/java/com/puppycrawl/tools/checkstyle/api/Scope.java#L64

When using the type AccessModifierOption instead, we would either need to use the same subscope semantics again (thus reproducing the original issue), or we could not configure checkstyle to e.g. check all variables, regardless of scope. Instead, we could only set one single access modifier, which should be checked.

In https://github.com/checkstyle/checkstyle/issues/3675#issue-197441645 it says

Looks like scope is counted by more visible and less visible. But should be simply by matching.

I thought this should apply here too. If this is not the case, how else could the original issue be solved?

@martin-mfg It sounds reasonable, and I like this changes. What bothers me is that it is breaking compatibility and issue is quite old, may be there are some nuances.

@romani @rnveach what do you think about #3183?

@martin-mfg , I created new issue https://github.com/checkstyle/checkstyle/issues/9280 please update reference in your commit message to new issue.

@romani A PR for this issue is already open in #7418 quite some time ago. Can 2 PR on same issue remain open?

PS: Fine now 👍

regarding red CI:

I'm not allowed to view any output from wercker, I only get "Hi, 401 - Unauthorized" when going there. Please tell me what is failing there.

If I understand correctly, two of the CI jobs try running checkstyle on the pmd project and on the xwiki project. They fail, because the changes I made were breaking changes. I.e. scope doesn't exist anymore for JavadocVariableCheck. How can I make breaking changes pass these CI jobs?

TeamCity fails because my switch statement here, which converts from Scope to AccessModifierOption, doesn't need and doesn't have cases for Scope.NOTHING and Scope.ANONINNER. What's your preferred way to handle this situation? (suppress the warning; add cases and make the method public to achieve test coverage; add cases and use mocking to achieve test coverage; ...?)

@martin-mfg This PR will need changes similar to https://github.com/checkstyle/checkstyle/pull/7418 for the CI. This wercker is failing on NoErrorTest - Spring Cloud GCP.

@romani I recommend we merge the others first since their CI is more along towards passing.

@martin-mfg, please pause work on this for a bit, we are close to merge state for #7418 by @rnveach

@martin-mfg, please rebase, related PR is merged

Regarding the travis CI error, I created PR https://github.com/checkstyle/xwiki-commons/pull/15.

On the one hand https://github.com/xwiki/xwiki-commons/pull/140 was opened directly in the original xwiki repo, on the other hand https://github.com/checkstyle/xwiki-commons/pull/14 was opened in the fork. Please tell me if I should redirect my own PR to the original xwiki repo.

@nmancus1 @romani, Is there anything I can do to get https://github.com/checkstyle/xwiki-commons/pull/15 turned into a new branch in checkstyle/xwiki-commons? I think that would be the next step necessary to merge this PR here.

please rebase to our latest code.

I used your commit to make https://github.com/checkstyle/xwiki-commons/tree/checkstyle_9280 please update travis script to do git checkout checkstyle_9280 to use your update in CI scripts.

wercker is failing on ./.ci/wercker.sh no-error-spring-cloud-gcp config needs to be updated also. but I see that configuration of such project use different project https://github.com/spring-io/spring-javaformat/blob/a2cd33bba8397f0f8f76a5278f8439355b11ae55/src/checkstyle/checkstyle.xml#L94-L96 it is will be very complicated to update it. Please send PR to this file(spring-io/spring-javaformat) and lets disable gcp project in wercker with comment above until <link to your PR> . After we release your fix they can merge a PR.

@martin-mfg , ping. Let me know if something is not clear.

@romani I updated the travis and wercker builds and also created https://github.com/spring-io/spring-javaformat/pull/274, as you requested above. I also created https://github.com/xwiki/xwiki-commons/pull/143 as you requested in https://github.com/checkstyle/xwiki-commons/pull/15.

I further created https://github.com/checkstyle/build-tools/pull/2 as a prerequisite to make the semaphore build work.

What happens to https://github.com/checkstyle/xwiki-commons/pull/15 now? Should I close it?

The xwiki/travis build still fails. After updating the checkstyle config, the problem is now different, and I guess it could be resolved by merging the latest https://github.com/xwiki/xwiki-commons/tree/master into https://github.com/checkstyle/xwiki-commons/tree/checkstyle_9280.

Github, rebase

@martin-mfg , if you allow maintaners to change your repo, it would be awesome

problem: https://github.com/checkstyle/checkstyle/runs/2905546480?check_suite_focus=true#step:6:24

sorry for being slow to reply to you.

Hi @romani, if I understand correctly, the access you requested is set by the checkbox in the bottom right corner here:

This has always been checked for this PR, right from when I created the PR. So it seems something else is going wrong. Anyway, I just rebased manually.

Github, rebase

@martin-mfg , please review failure of https://checkstyle.semaphoreci.com/jobs/42f5e408-f8e4-43d7-be56-ea2118570d69 did we miss some update ?

for xwiki failure: Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact org.xwiki.commons:xwiki-commons-pom:pom:13.7-SNAPSHOT

we do not have this problem in master build .... Do we need to get latest from xwiki and rebase our branch ?

@romani Regarding semaphoreci, I need someone to create a branch from https://github.com/checkstyle/build-tools/pull/2 please. Regarding xwiki, the main difference between the master branch and my branch seems to be that the pipeline has been reconfigured. Therefore I just rebased my branch. CircleCI is still running, let's see what the result will be.

@romani Regarding xwiki, I fixed the failing build by merging in the latest commits from the master branch at https://github.com/xwiki/xwiki-commons.

To demostrate the build is working, I temporarily changed the checkstyle pipeline task to use checkout_from "-b updated_checkstyle_9280 https://github.com/martin-mfg/xwiki-commons.git" (my repo) instead of checkout_from "-b checkstyle_9280 https://github.com/checkstyle/xwiki-commons.git" (checkstyle owned repo). I guess the next step is to create a new branch with the updated xwiki code in https://github.com/checkstyle/xwiki-commons. To this end I created https://github.com/checkstyle/xwiki-commons/pull/18. Or if you can update the master branch in https://github.com/checkstyle/xwiki-commons.git, I'll rebase https://github.com/checkstyle/xwiki-commons/pull/15 then. Once this is done, I need to change back the checkstyle pipeline task.

we recently run into edge case with scope property but for another Check - https://github.com/checkstyle/checkstyle/issues/13749 , and I am not sure how accessModifiersOption is going to handle it.

@romani this PR has been hanging for three years, we need to proceed with review or close it.