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

Update all AbstractChecks to log DetailAST #662

Closed
rnveach opened this Issue May 27, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@rnveach
Copy link
Contributor

rnveach commented May 27, 2018

All AbstractChecks should use log(DetailAST ast, String key, Object... args) method to log DetailAST.

Similar to checkstyle/checkstyle#4830

@rnveach rnveach added the approved label May 27, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue May 27, 2018

romani added a commit that referenced this issue May 27, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue May 27, 2018

rnveach added a commit that referenced this issue May 27, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue May 27, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue May 27, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue May 27, 2018

@rnveach

This comment has been minimized.

Copy link
Contributor

rnveach commented May 27, 2018

The only checks left should be ForbidCCCommentsInMethods (whole check has to be rewritten) and LineLengthExtended (not sure how to convert it over).

romani added a commit that referenced this issue May 27, 2018

romani added a commit that referenced this issue May 27, 2018

romani added a commit that referenced this issue May 27, 2018

@romani

This comment has been minimized.

Copy link
Member

romani commented May 27, 2018

ForbidCCCommentsInMethods

we can put violation on any AST that exists before comment.

LineLengthExtended

we can put violation on first AST in line.

@rnveach

This comment has been minimized.

Copy link
Contributor

rnveach commented May 27, 2018

put violation on any AST that exists before comment
put violation on first AST in line

Obviously, but finding these ASTs is the problem. We have no util that finds AST right before line or on line, etc... Even if we did have such a utility, it would almost require to reparse a chunk of the tree.

@rnveach

This comment has been minimized.

Copy link
Contributor

rnveach commented Jun 7, 2018

MultipleStringLiteralsExtendedCheck also needs to be converted. I confirmed that these are the only 3 left.

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

romani added a commit that referenced this issue Jun 7, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

rnveach added a commit to rnveach/sevntu.checkstyle that referenced this issue Jun 7, 2018

@rnveach

This comment has been minimized.

Copy link
Contributor

rnveach commented Jun 8, 2018

@romani Once ForbidCCCommentsInMethods is merged, I feel we should just move LineLengthExtendedCheck to a new issue and close this one.

There is no easy way to convert it. The check is comment aware and empty line aware. It calls getLines() and can see everything in the file. To me this check seems like it is an example of a FileSet check with a AbstractCheck that acts as a holder. I know we don't like holders, but I don't see any other good way.

romani added a commit that referenced this issue Jun 8, 2018

@romani

This comment has been minimized.

Copy link
Member

romani commented Jun 8, 2018

to a new issue and close this one.

agree.

I know we don't like holders, but I don't see any other good way.

lets keep it as is, if smb care about it, it will be updated. There will be more intensive to update it.

@rnveach

This comment has been minimized.

Copy link
Contributor

rnveach commented Jun 8, 2018

Closing this as all other checks are done and last one was moved to new issue.

@rnveach rnveach closed this Jun 8, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

kariem added a commit to kariem/sevntu.checkstyle that referenced this issue Jul 26, 2018

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