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 #12210: Add method to ignore unstable checker framework violations #12215

Merged

Conversation

Vyom-Yadav
Copy link
Member

Resolves #12210

@romani
Copy link
Member

romani commented Sep 18, 2022

Codenarc failing

@Vyom-Yadav Vyom-Yadav force-pushed the fixCheckerFrameworkUnstableErrorMethod branch 2 times, most recently from 4fb8b76 to da8c139 Compare September 18, 2022 17:16
@Vyom-Yadav
Copy link
Member Author

Codenarc failing

Done.

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Items:

.ci/checker-framework.groovy Outdated Show resolved Hide resolved
.ci/checker-framework.groovy Outdated Show resolved Hide resolved
.ci/checker-framework.groovy Show resolved Hide resolved
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

One more:

.ci/checker-framework.groovy Outdated Show resolved Hide resolved
@Vyom-Yadav Vyom-Yadav force-pushed the fixCheckerFrameworkUnstableErrorMethod branch from 172aeb9 to b4f8be9 Compare September 19, 2022 13:01
@romani
Copy link
Member

romani commented Sep 19, 2022

Codenarc again

@Vyom-Yadav Vyom-Yadav force-pushed the fixCheckerFrameworkUnstableErrorMethod branch from b4f8be9 to b2bd6b6 Compare September 19, 2022 13:26
@Vyom-Yadav
Copy link
Member Author

Codenarc again

Done.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge

@romani romani merged commit 701bd65 into checkstyle:master Sep 19, 2022
return i
if (this.isUnstable() || other.isUnstable()) {
final String messageWithoutLineNumber = getMessage().replaceAll('\\d+', '')
final String thatMessageWithoutLineNumber = other.getMessage().replaceAll('\\d+', '')
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't we do this in the constructor? This is suppose to be an immutable class.

Would also save on execution time, as this gets run probably every time a new entry gets added to the set for nearly everything in the set multiple times.

We obviously don't care about the numbers anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is suppose to be an immutable class

It still is, the original field is not modified.

Would also save on execution time, as this gets run probably every time a new entry gets added to the set for nearly everything in the set multiple times.

It will only run for unstable mutations, only a few of them are there.

We obviously don't care about the numbers anyways.

I feel it would be better if we don't modify the original error itself.

return i
if (this.isUnstable() || other.isUnstable()) {
final String messageWithoutLineNumber = getMessage().replaceAll('\\d+', '')
final String thatMessageWithoutLineNumber = other.getMessage().replaceAll('\\d+', '')
Copy link
Member

Choose a reason for hiding this comment

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

Will numbers that were an issue always be 3 digits? Maybe consider changing the regexp to just match 3 digit numbers.
Also why not match the # too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will numbers that were an issue always be 3 digits?

No, they are not, see the updated issue description, we have entries like temp-var-20910

Also why not match the # too?

Not possible as we have entries like temp-var-20910, also if we could do it, there is no point in doing it, would increase the complexity of regex unnecessarily.

@Vyom-Yadav
Copy link
Member Author

I think we breach the equals and hashcode contract here, I am investigating it.

@rnveach
Copy link
Member

rnveach commented Sep 20, 2022

If you think this is broken, I recommend adding a print out of full actual so it can be viewed in the logs and debugged easier.

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.

Add method to ignore unstable checker framework violations.
4 participants