DM-37339: Fix typing on Config class #805
Conversation
timj
force-pushed
the
tickets/DM-37339
branch
2 times, most recently
from
d1a9e37
to
a039386
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #805 +/- ##
==========================================
- Coverage 85.61% 85.61% -0.01%
==========================================
Files 266 266
Lines 35309 35314 +5
Branches 7430 7433 +3
==========================================
+ Hits 30230 30234 +4
Misses 3749 3749
- Partials 1330 1331 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
andy-slac
force-pushed
the
tickets/DM-37339
branch
from
6e71cc0
to
491eb2e
Compare
| # Remote resource check might be expensive | ||
| if specific.exists(): | ||
| found = specific | ||
| # TODO: do we need `break` here? |
There was a problem hiding this comment.
@timj, I think that we might want a break here, don't we?
There was a problem hiding this comment.
Yes, looks like it should break out the loop once it has found something in the search path...
There was a problem hiding this comment.
OK, I'll add a break and re-run Jenkins.
| # Remote resource check might be expensive | ||
| if specific.exists(): | ||
| found = specific | ||
| # TODO: do we need `break` here? |
There was a problem hiding this comment.
Yes, looks like it should break out the loop once it has found something in the search path...
| key, | ||
| ] | ||
| # Do not try to guess. | ||
| raise TypeError(f"Provided key [{key}] neither str nor iterable.") |
There was a problem hiding this comment.
I'm assuming this is okay. mypy didn't like the fallback code and so it was easier for me to change it to a raise. If jenkins passes it's likely okay.
| elif isinstance(key, Iterable): | ||
| return list(key) | ||
| return [k for k in key] |
There was a problem hiding this comment.
I was unsure about this one as well. It may be that it can go back to being list(key) (which I assume is more efficient?)
There was a problem hiding this comment.
list is likely more efficient, and it should work OK, I'll put it back.
| keys = [ | ||
| name, | ||
| ] | ||
| keys = [cast(str, name)] |
There was a problem hiding this comment.
This is probably the right thing to do but in theory name could be a tuple -- the trick is that if in works directly there is no reason to try anything clever.
There was a problem hiding this comment.
But self._data keys are strings, right? Or at least this is how it is annotated.
There was a problem hiding this comment.
Okay. Yes, name is anything but in this case we are looking in the internal dict and so yes it's only going to work if it's a string.
| @@ -623,8 +621,9 @@ def __getitem__(self, name): | |||
| # all further cleverness. | |||
| found_directly = False | |||
| try: | |||
| data = self._data[name] | |||
| found_directly = True | |||
| if isinstance(name, str): | |||
There was a problem hiding this comment.
It does seem painful to have to do an isinstance check even though the dict lookup would fail if the item wasn't a string but that's the price we pay for mypy. Especially when this short circuit was there as an optimization to let the fast thing be fast.
There was a problem hiding this comment.
Well, it is still an optimization, but for non-string case. I think checking the type first should be as quick or quicker than checking the non-string key in a dict.
andy-slac
force-pushed
the
tickets/DM-37339
branch
from
e6ffff0
to
1365f07
Compare
Checklist
doc/changes