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.

Sign up for GitHub

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

Jump to bottom

feat: allow stack trace suppression (#266) #275

Merged
merged 2 commits into from
Jun 18, 2020
Merged

feat: allow stack trace suppression (#266) #275

merged 2 commits into from
Jun 18, 2020

Conversation

chriswhite199
Copy link
Contributor

No description provided.

@chriswhite199 chriswhite199 changed the title #266 - Allow stack trace suppression feat: Allow stack trace suppression May 28, 2020
@chriswhite199 chriswhite199 changed the title feat: Allow stack trace suppression feat: allow stack trace suppression (#266) May 28, 2020
KengoTODA
KengoTODA previously approved these changes Jun 6, 2020
View reviewed changes
Copy link
Member

@KengoTODA KengoTODA left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I'll ask other contributor to merge, for double-check.

We have no issue nor feedback regarding this change, so I'm personally not sure how (in which case) this feature helps our users.

@chriswhite199
Copy link
Contributor Author

so I'm personally not sure how (in which case) this feature helps our users.

Type-A developers like myself see stack traces as something gone wrong in my gradle build - the reports file detail what went wrong, and gradle can still return a non 0 RC if ignoreFailures is set, so its just a feature to remove the overly alarming stack trace if the user choses to do so.

jscancella
jscancella reviewed Jun 13, 2020
View reviewed changes
src/functionalTest/groovy/com/github/spotbugs/snom/StandardFunctionalTest.groovy Outdated
}
@Unroll
def 'build does not show stack traces when bugs are found with `showStacktraces = true` (Worker API? #isWorkerApi)'() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment here doesn't match your build file on line 273. Either update the comment or change the test to match the comment.

Copy link
Contributor Author

@chriswhite199 chriswhite199 Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should be fixed in 7947270. Do you want a squashed single commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, that should be d14465d

jscancella
jscancella requested changes Jun 13, 2020
View reviewed changes
Copy link
Contributor

@jscancella jscancella left a comment

Choose a reason for hiding this comment

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

LGTM except for the one test comment that I pointed out is inconsistent with the actual test. Once that is fixed I vote we merge it.

jscancella
jscancella approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@jscancella jscancella left a comment

Choose a reason for hiding this comment

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

LGTM. I personally don't like ternary operators, but I couldn't think of a reasonable alternative without making it just as ugly for where we print out the stacktrace.

@KengoTODA KengoTODA merged commit 2c7e412 into spotbugs:master Jun 18, 2020
@github-actions
Copy link

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cmuchinsky cmuchinsky mentioned this pull request Jun 18, 2020
Closed
@KengoTODA KengoTODA mentioned this pull request Jun 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jscancella jscancella jscancella approved these changes

@KengoTODA KengoTODA KengoTODA approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chriswhite199 @KengoTODA @jscancella