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 12 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'm not 100% sure this can be optional.
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 should probably be public?
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.
Assuming this class is public, we want to keep guava types off the surface of the classes. This should be a
List<String>
and we'd document that the implementation is immutable so they shouldn't try to modify it.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 missed this comment a while back. The reason I'm using ImmutableList<> is based on recommendation from AutoValue docs https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#-use-a-collection-valued-property. I want getMembers() to support a builder helper as well in case it's easier for users.
WDYT?
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 ImmutableList and now using List in the surface while still supporting helper methods addMembers and removeMembers.
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.
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.
nit - I would extract some of these out into variables to make it a little clearer
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 should probably be public?
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.