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.
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
docs: add sample for revoking dataset access #778
docs: add sample for revoking dataset access #778
Changes from 40 commits
0abac4c
d699bec
98b245d
f1adee7
e729bbd
b80c4bf
2960b48
02bd2b4
fa0e48c
98bb4a2
8c4bc7a
5197d0f
cfb3805
4be9760
0a57eba
d4672f2
b9e35aa
b076972
4ec3512
15d84b1
2209d8e
0da2301
77066a5
32d9b71
b7e91eb
318242d
5fe08d0
1c4469b
fe66df2
fbcc09f
4234450
2b82c45
2ea0f4e
6bbdc33
a493adc
575260a
27d8170
adc1c76
62ec25e
072b785
dade3b8
c74022e
9da6de6
ede9158
05a4f19
6c04f28
d030e21
023eb96
6d9f274
3fd6856
6ce391d
04497a1
2e98eca
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.
@parthea Looks like this is a reversion by owlbot.
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.
At first glance, this change wasn't made by owlbot. See cfb3805#diff-80e5f0b5c748e24702257bf661911a6c240d81c82a5029056391211591a4a5c9
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.
@loferris Please revert the changes to this file.
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: In the filename, "access" and "permissions" are redundant. Let's call it
dataset_access_test.py
.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.
Done in latest commit!
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.
We should add a comment saying that the developer will need to set this to the correct type.
Although, perhaps we can delete this whole line 32-36? A code sample should only contain the logic necessary for the developer to understand the concept we're trying to teach them.
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 took it out!
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.
Should this be
entity_id=entity_id
?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.
Makes sense to me!
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 a bit confused as to why we'd add the entry and then remove it on line 42. Perhaps a copy-paste error?
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 think it's a redundancy - I was adding a specific entry to alter but I think that'll be covered in the test case and sort of muddies the water in the sample itself. I'll refactor.
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 line confuses me. It gets overwritten in the next line of code, so looks unnecessary. Possibly leftover from a debugging?
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.
Likewise here, let's use the "save the original, then overwrite it" pattern.
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.
Looks like we missed this one.
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 needs a comment explaining the various options available, or at least a link to the docs where these are described.
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.
Added!
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 we want to use the
entity_id
fixture here? If so, we should add a variable above.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.
Added, changed test as well!