-
Notifications
You must be signed in to change notification settings - Fork 35
schemaview.py: check for a valid slot def before calling a function #480
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
Conversation
…ling various slot range functions'
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
+ Coverage 78.20% 78.24% +0.03%
==========================================
Files 52 52
Lines 4525 4533 +8
Branches 985 988 +3
==========================================
+ Hits 3539 3547 +8
Misses 768 768
Partials 218 218 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sujaypatil96
left a comment
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.
Adding guardrails to check that the input to some slot details fetcher methods is in fact an object of type SlotDefinition.
Silvanoc
left a comment
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, only optional improvement proposal
| if not slot or not isinstance(slot, SlotDefinition): | ||
| err_msg = "A SlotDefinition must be provided to generate the slot applicable range elements." | ||
| raise ValueError(err_msg) | ||
|
|
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.
Wouldn't it make sense to have a function for this purpose, since the same validation is being applied in multiple places?
I mean something like
def _validate_slot(slot, err_sfx) -> None:
if not slot or not isinstance(slot, SlotDefinition):
raise ValueError(
f"A SlotDefinition must be provided to generate the {err_sfx}."
)BTW you are typically optimizing. I don't get why you use the variable err_msg just for a single use.
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.
Feel free to resolve comment, and merge, if desired
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.
the err_msg thing is to make the stack trace clearer -- otherwise you get an extra line:
https://docs.astral.sh/ruff/rules/raw-string-in-exception/
You are absolutely right about optimising by creating a generic function. I don't know why I didn't do it - end of the workday, maybe?
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're right, I forgot that recommendation 😄
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 going to investigate options because as soon as I see a verification function like that, I want to make it more generic. I need to get my core work done and then I'll dedicate some time to thinking/researching argument validation.
Adding a little bit of "insurance" to some slot range functions to ensure that they check the input is the correct type before running off and performing any functions with it.