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 30 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.
This could even take
Iterable<String>
if you want to be most general.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.
Thanks, addressing.
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.
Is this used from anywhere? Do you need it? Generally, builders don't need getters.
Same for
getCondition()
.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.
removed to reduce surface size. At the moment they were convenience but I'd like to get more developer input before adding unnecessary methods to the public surface.
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.
Do you expect this to be called other than from
addMembers(String...)
andremoveMembers(String...)
? If not, make this have default access.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.
No, good point.
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.
To avoid this constraint,
newBuilder()
could callsetMembers(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.
Sweet cohesive behavior. ty!
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.
It wouldn't make sense to call this with no arguments, right? If you change it to the following, then that wouldn't even compile.
You can then use Guava's
Lists.asList(member, moreMembers)
(a good static import method) to combine those two into one list to add.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.
That should work, no?
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.
Tiny nit:
Predicates.not
andPredicates.in
look good when they're statically imported.Arrays.asList
too.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 need to make a copy if
setMembers(List)
takesIterable
.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 are these nullable?
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.
Good point, no reason other than my first attempt with AutoValue. removing.