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

Rename hasArgs to containsArgs #176

Merged
merged 4 commits into from
Jul 16, 2019
Merged

Rename hasArgs to containsArgs #176

merged 4 commits into from
Jul 16, 2019

Conversation

pkoenig10
Copy link
Member

@pkoenig10 pkoenig10 commented Jul 15, 2019

Before this PR

LoggableExceptionAssert#hasArgs only checks whether the given args are contained in the actual args. The name of this method in misleading, as many users probably assume that the method checks that the args are exactly equal. This leads to weaker tests than users may have intended.

This is particularly confusing because ServiceExceptionAssert#hasArgs has the same name but does check that the args are exactly equal.

After this PR

LoggableExceptionAssert#hasArgs is marked as deprecated in favor of a new method, ``LoggableExceptionAssert#containsArgs`. This should more clearly signal the behavior of this method to users.

This also more closely matches the naming convention of iterables asserts in AssertJ.

@pkoenig10 pkoenig10 requested a review from a team as a code owner July 15, 2019 19:45
@dansanduleac
Copy link
Contributor

dansanduleac commented Jul 16, 2019

Makes sense, would you consider adding a refaster rule as well?
Should be as easy as a new published project safe-logging-refactorings with

apply from: "$rootDir/gradle/publish-jar.gradle"
dependencies {
    implementation 'com.google.errorprone:error_prone_refaster'
}

@ferozco
Copy link

ferozco commented Jul 16, 2019

👍

@pkoenig10
Copy link
Member Author

@ferozco looks like policy-bot didn't accept your approval because you added a merge commit

@carterkozak
Copy link
Contributor

👍 (haven't reviewed, but I trust @ferozco :-)

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.

4 participants