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 #461: Field single declaration check implemented #811

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yaziza
Copy link
Member

@yaziza yaziza commented Apr 14, 2020

Comment on lines +88 to +89
final DetailAST findFirstToken = ast.findFirstToken(TokenTypes.TYPE);
final FullIdent ident = CheckUtil.createFullType(findFirstToken);
Copy link
Member Author

@yaziza yaziza Apr 14, 2020

Choose a reason for hiding this comment

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

Not sure if we should test findFirstToken and ident against NPE. A variable definition should always have a TYPE and IDENT right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Regression would should if an exception is possible.

@yaziza yaziza force-pushed the 461-field-declaration-count branch 4 times, most recently from 7e6921b to 7cd43be Compare April 14, 2020 08:51
@yaziza yaziza force-pushed the 461-field-declaration-count branch from 7cd43be to e66589d Compare April 14, 2020 09:15
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 96.343% when pulling e66589d on yaziza:461-field-declaration-count into bd5ca6b on sevntu-checkstyle:master.

@yaziza
Copy link
Member Author

yaziza commented Apr 14, 2020

Unfortunately, i was not able to fix the build of all-sevntu-checks-contribution

https://travis-ci.org/github/sevntu-checkstyle/sevntu.checkstyle/jobs/674768175

Any hint ?

@rnveach
Copy link
Contributor

rnveach commented May 19, 2020

Any hint ?

A PR to contribution report has to be made to add the check. It can only be merged after this PR is merged, so as long as it is the only error, it is fine for it to stay.

public void visitToken(DetailAST ast) {
if (SevntuUtil.matchesFullyQualifiedName(ast, fullyQualifiedClassName)) {
if (hasPreviouslySeenClass) {
log(ast.getLineNo(), MSG_KEY, fullyQualifiedClassName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please give it the full AST.


@Override
public int[] getDefaultTokens() {
return new int[] {TokenTypes.VARIABLE_DEF };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this being enough. You are not checking imports so your implementation is very susceptible to name conflicts.

@rnveach
Copy link
Contributor

rnveach commented Oct 10, 2022

@yaziza ping
Do you plan to finish this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants