-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
chore(security): Make get_database_perm/get_dataset_perm return optional #24046
chore(security): Make get_database_perm/get_dataset_perm return optional #24046
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24046 +/- ##
=======================================
Coverage 68.22% 68.22%
=======================================
Files 1941 1941
Lines 75261 75162 -99
Branches 8168 8134 -34
=======================================
- Hits 51344 51280 -64
+ Misses 21828 21803 -25
+ Partials 2089 2079 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2797a63
to
8d7d5c0
Compare
8d7d5c0
to
08547b2
Compare
b8f8fcc
to
08547b2
Compare
@michael-s-molina apologies for the delay in responding to your comments. Regarding replacing from __future__ import annotations import and thus this syntax is not supported. My sense/preference is that this file (alongside others) should be updated in a different PR to keep this change to a minimum. |
08547b2
to
4392911
Compare
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.
LGTM
SUMMARY
Per here perms can be
None
and thus these return types (which could be overloaded) should beOptional[str]
rather thanstr
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION