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

ban assert #1810

Merged
merged 2 commits into from
Jun 24, 2021
Merged

ban assert #1810

merged 2 commits into from
Jun 24, 2021

Conversation

carterkozak
Copy link
Contributor

Before this PR

assert was allowed.

After this PR

==COMMIT_MSG==
Introduce BadAssert to ban assert statements in favor of better alternatives.
==COMMIT_MSG==

Possible downsides?

May block builds? That's rather the point.

@changelog-app
Copy link

changelog-app bot commented Jun 23, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Introduce BadAssert to ban assert statements in favor of better alternatives.

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak
Copy link
Contributor Author

I've intentionally opted against automatically applying the suggested fixes given how rare they are, and some subtle uses (relying on side-effects based on whether or not asserts are enabled) could be broken by automatic changes.

fix.replace(
tree,
String.format(
"if (!%s) { throw new IllegalStateException(%s); }",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These can result in funny code when the input is assert !value -> if (!!value) { throw.... Not worth optimizing here imo because we have other tools to catch and resolve those, and the fixes are only for documentation, not enabled by default.

void testNonConstantStringDescription() {
fix().addInputLines(
"Test.java",
// format-hint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatter puts all these onto one line without a well-placed comment. Some day we'll have text blocks.

Copy link
Contributor

@CRogers CRogers left a comment

Choose a reason for hiding this comment

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

Nice!

@bulldozer-bot bulldozer-bot bot merged commit 645ba76 into develop Jun 24, 2021
@bulldozer-bot bulldozer-bot bot deleted the ckozak/ban_assert branch June 24, 2021 10:36
@svc-autorelease
Copy link
Collaborator

Released 3.94.0

bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Jun 24, 2021
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 3.94.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Introduce `BadAssert` to ban `assert` statements in favor of better alternatives. | palantir/gradle-baseline#1810 |



To enable or disable this check, please contact the maintainers of Excavator.
This was referenced Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants