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

Improve StreamMapToOptionalGet Refaster template documentation #203

Merged
merged 2 commits into from
Aug 19, 2022
Merged

Improve StreamMapToOptionalGet Refaster template documentation #203

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fc399f
Update `StreamMapToOptionalGet` Javadoc
rickie Aug 18, 2022
493db87
Tweak
Stephan202 Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ...one-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,16 @@ Stream<T> after(Stream<Optional<T>> stream) {
}
}

/** Within a stream's map operation unconditional {@link Optional#get()} calls can be avoided. */
// XXX: An alternative approach is to `.flatMap(Optional::stream)`. That may be a bit longer, but
// yield nicer code. Think about it.
/**
* Within a stream's map operation unconditional {@link Optional#orElseThrow()} calls can be
* avoided.
*
* <p><strong>Warning:</strong> this rewrite rule is not completely behavior preserving. The
Copy link
Contributor

Choose a reason for hiding this comment

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

In most cases since we're in a stream already I suppose flattening the Optional would be the user's intention, but I wonder how we could also convey/educate users with the changes that are made for them. Now, the process is such that if they don't like a change, they will ping @rickie to ask, but it might be nice to also think of an automated solution for templates similar to how there's documentation for bug patterns.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, have been pondering the same. Ideally the Refaster check can emit custom error messages, just like other BugCheckers. We might be able to use annotations for this; TBD.

Next to that we should generate Github pages (or equivalent) and link to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also bounced a few ideas offline just now with @rickie. Annotations would be super nice -- we could also use it to indicate whether some things are behavior preserving or not, and I'm sure we can think of many more nice categories. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a lot of cool ideas and options. Would be really nice to get stuff like that done 😍.

* original code throws an exception if the mapping operation does not produce a value, while the
* replacement does not.
*/
// XXX: An alternative approach is to use `.flatMap(Optional::stream)`. That may be a bit longer,
// but yields nicer code. Think about it.
abstract static class StreamMapToOptionalGet<T, S> {
@Placeholder
abstract Optional<S> toOptionalFunction(@MayOptionallyUse T element);
Expand Down