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

Improve naming of assorted Refaster templates and associated tests #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rickie
Copy link
Member

@rickie rickie commented Jan 6, 2022

This PR contains code of #25 . We want to prepare the Refaster templates such that we can easily add the annotations @Template{,Collection} in the appropriate places.
In the afternoon we talked about the reordering of some Refaster templates. I considered doing that in a second commit, but checked the files again, and saw that there is no alphabetical ordering currently, so I left it out.
Also, in the future we most likely want to automate the ordering with another BugPattern 😉, so did not bother to do it right now.

Note: for some Refaster templates our "current" naming scheme does not suffice.

@rickie rickie requested a review from Stephan202 January 6, 2022 19:27
@@ -657,8 +656,7 @@ void after(Map<K, V> map) {
}
}

// XXX: Find a better name.
static final class AssertThatMapIsNotEmpty2<K, V> {
static final class AssertThatIsNotEmpty<K, V> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

Copy link
Member

Choose a reason for hiding this comment

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

Likewise :)

@@ -615,8 +615,7 @@ void after(AbstractMapAssert<?, ?, K, V> mapAssert) {
}
}

// XXX: Find a better name.
static final class AssertThatMapIsEmpty2<K, V> {
static final class AssertThatIsEmpty<K, V> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely we want different names for this in the future, but for now I followed the naming scheme.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, AssertThatMapIsEmpty would be better (and possible now that the other method is renamed).

@Stephan202 Stephan202 force-pushed the rossendrijver/improve_template_names branch from 07a25cf to e747a47 Compare April 10, 2022 13:47
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Rebased and added a commit. Seems to require a bit more thinking 🤔

@@ -615,8 +615,7 @@ void after(AbstractMapAssert<?, ?, K, V> mapAssert) {
}
}

// XXX: Find a better name.
static final class AssertThatMapIsEmpty2<K, V> {
static final class AssertThatIsEmpty<K, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, AssertThatMapIsEmpty would be better (and possible now that the other method is renamed).

@@ -657,8 +656,7 @@ void after(Map<K, V> map) {
}
}

// XXX: Find a better name.
static final class AssertThatMapIsNotEmpty2<K, V> {
static final class AssertThatIsNotEmpty<K, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise :)

@@ -186,7 +186,7 @@ OptionalDouble after(DoubleStream stream) {
}

/** Prefer {@link DoubleStream#noneMatch(DoublePredicate)} over more contrived alternatives. */
static final class DoubleStreamNoneMatch {
static final class DoubleStreamNoneMatchDoublePredicate {
Copy link
Member

Choose a reason for hiding this comment

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

Here and below: I wonder whether in this case the original naming is more natural, since the lambda expression below also represents a DoublePredicate. (Or maybe we can do something more creative 🤔.)

@Stephan202 Stephan202 force-pushed the rossendrijver/improve_template_names branch from e747a47 to 395d266 Compare June 4, 2022 17:25
@rickie
Copy link
Member Author

rickie commented Sep 2, 2022

I'd suggest to close this PR.

Once we work on the BugPattern that'll enforce a specific naming scheme for Refaster templates, we can use the ./apply-error-prone-suggestions.sh to do the work for us. It's not worth the effort now to get this PR merged, especially considering that there are still some unknowns w.r.t. the final naming scheme.

WDYT @Stephan202 ?

@Stephan202
Copy link
Member

I'm fine closing this PR as-is, though OTOH such a checker could save us quite some time when open-sourcing the code base. How was the current changeset generated, and could we use that as the foundation of a new checker?

@rickie
Copy link
Member Author

rickie commented Sep 5, 2022

The current changeset was done manually (according to the ideas we had at the time about the naming pattern).
I wrote some stuff down after we talked about a naming pattern a few times and we should use that as foundation for the new check. Therefore I think these changes do not add a lot of value.

@rickie rickie self-assigned this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants