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

Add DoNotUseIsOneAssertions rule #37

Merged
merged 2 commits into from
May 23, 2024
Merged

Add DoNotUseIsOneAssertions rule #37

merged 2 commits into from
May 23, 2024

Conversation

nakulpathak3
Copy link
Contributor

@nakulpathak3 nakulpathak3 requested a review from travisMiehm May 21, 2024 18:45
@nakulpathak3 nakulpathak3 force-pushed the nakul.doNotUseIsOne branch from 6806a47 to ff7a853 Compare May 21, 2024 18:59
@travisMiehm travisMiehm requested a review from a team May 22, 2024 18:57
@nakulpathak3
Copy link
Contributor Author

PR to remove the inverse rule from backend - https://github.com/Faire/backend/pull/146826

{ DoNotUseIsOneAssertions(it) },
) {
@Test
fun `using isOne causes failure`() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a test for calling isOne() with a receiver context that is not an assertion method?

@@ -42,6 +43,7 @@ internal class FaireRulesProvider : RuleSetProvider {
DoNotUseHasSizeForEmptyListInAssert(config),
DoNotUseIsEqualToWhenArgumentIsOne(config),
DoNotUseIsEqualToWhenArgumentIsZero(config),
DoNotUseIsOneAssertions(config),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this rule directly contradicts DoNotUseIsEqualToWhenArgumentIsOne, do we have to worry about both rules being enabled by default? I think the official detekt team usually adds rules disabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the way to have it disabled by default to just not add it to this file? I'm dropping that one for backend in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Talked offline, we'll let users of these rules just update their config file when they update the version.

@nakulpathak3 nakulpathak3 force-pushed the nakul.doNotUseIsOne branch from 76eca40 to 4539950 Compare May 23, 2024 15:12
@nakulpathak3 nakulpathak3 merged commit 8a7de80 into main May 23, 2024
3 checks passed
@nakulpathak3 nakulpathak3 deleted the nakul.doNotUseIsOne branch May 23, 2024 15:22
nakulpathak3 added a commit that referenced this pull request May 23, 2024
Following these steps from @Adriel-M to get my rule available in the
backend repo - #37

To do a release you must:
* [This PR] bump version here (after your change gets merged in):
https://github.com/Faire/faire-detekt-rules/blob/main/gradle.properties
* Run this workflow
https://github.com/Faire/faire-detekt-rules/actions/workflows/release.yml
(at this point, this is going to publish to maven central :nodders: )
* (optional but you should) git tag that commit to the same version in 1
* create a release here:
https://github.com/Faire/faire-detekt-rules/releases
* If you want to test out the rule on backend before actually creating a
release, I did create this write up in our other repo:
https://github.com/Faire/sqldelight-cockroachdb-dialect?tab=readme-ov-file#using-a-snapshot-release
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.

2 participants