This repository has been archived by the owner on Dec 3, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: support conditional policies #110
feat: support conditional policies #110
Changes from 6 commits
67e52ce
84fcfea
145f7a0
ac1fd6e
2c8724b
c90dbb0
b0a617f
9dbfc2d
54ad076
ff49620
298b2be
aaebba4
a5f63ea
4873b73
c853a84
c64e55a
d2fab21
86cd863
085959d
174e8c4
14e1aac
fdb040a
42199d1
bbb708a
7f0e33e
108faec
79126b5
505f9bc
a89ef0c
f0b5085
9f5e600
8580f5a
9fe4358
8f48a15
615ba06
2b56641
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd very much recommend Javadoc for public types and public methods that aren't obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
@Nullable
instead of a non-nullableOptional<Condition>
?Either is okay, but it's interesting that you're using both patterns in the same class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I picked Nullable because it made more sense to me in this case. I'm not well versed in Optional's. Both patterns? Not sure I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought for some reason that you were also using
Optional
. Don't know why I thought that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just inline
emptyMembers
intosetMembers(ImmutableList.of())
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't really need to repeat this everywhere. It's better to use
@Nullable
for the hopefully few cases where null is accepted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think all of these should use
checkState
instead ofcheckArgument
so they throwIllegalStateException
.Throw
IllegalArgumentException
when the caller should have known that the arguments to this method are invalid.Throw
IllegalStateException
when the caller should have known that this method cannot be called given the state of the object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could return an
ImmutableSetMultimap
instead.