DM-31722: Clean up utility code usage #582
Conversation
Codecov Report
@@ Coverage Diff @@
## master #582 +/- ##
==========================================
- Coverage 83.62% 83.47% -0.16%
==========================================
Files 241 241
Lines 30347 30136 -211
Branches 4517 4497 -20
==========================================
- Hits 25379 25156 -223
- Misses 3781 3786 +5
- Partials 1187 1194 +7
Continue to review full report at Codecov.
|
timj
force-pushed
the
tickets/DM-31722
branch
4 times, most recently
from
81ae4f0
to
e514ba5
Compare
| records = [ButlerLogRecord.parse_raw(line) for line in isplit(serialized, newline) # type: ignore | ||
| if line] | ||
| # argument to isplit() [which can't have two different types | ||
| # simultaneously] so we have to duplicate some logic. |
There was a problem hiding this comment.
I'm curious what the mypy error was with the original code.
There was a problem hiding this comment.
It was complaining that Union[str, bytes] for the separator can't work for a str parameter that is being split because the T type can't then work out what the return type is. With the rewrite it knows that serialized is the same type as newline so can know what isplit returns. Fundamentally if a variable at any point has a Union type it can't be converted to a non-Union type without being assigned to a new variable.
There was a problem hiding this comment.
Fundamentally if a variable at any point has a Union type it can't be converted to a non-Union type without being assigned to a new variable.
I've definitely seen mypy "collapse" a union down to the remaining type(s) when one of them is rejected in other contexts (e.g. if you assert x is not None, it can tell that x is T in Optional[T]). But I guess there's not much value in trying to figure out why it couldn't do that here.
There was a problem hiding this comment.
I think adding 4 asserts (two in each if branch) might do it but I'm not sure if that's much of an improvement.
|
The code coverage reports only turn up for new code on this pull request. If I merge this PR and no-one else ever touches that code, codecov will never report any coverage problem. I added new code with an if clause that is not triggered by any of the tests and so that counts against this PR as being a drop in coverage. Codecov is complaining about this PR because there was a net decrease in code coverage because I added all these checks for mypy. I can ignore them and merge or I can create a few bad config files and try to create butlers from them. |
I am all for ignoring them here. I was just hoping we could ignore them forever (e.g. keep them from coming back again if we, say, |
|
If we black-formatted everything then codecov would absolutely lose its mind because of the 16% of the codebase that has no code coverage at all. We'd have to take that on the chin. I don't think it's unreasonable for codecov to warn you when you've edited a line but don't have any test code to show that the edit made a difference. |
Many utility routines have been relocated to the lsst.utils package. Remove them from daf_butler and use them.
It is not used and ButlerURI version is preferred.
os.makedirs is now safe
timj
force-pushed
the
tickets/DM-31722
branch
2 times, most recently
from
b69959c
to
b9f8c3e
Compare
mypy noted that doImport can return a Module and this was not expected in many cases. mypy also requires that there is a check on the return value from doImportType to ensure that it is of the correct class.
Mypy does not like isplit(str, str | bytes) so rearrange the logic to have distinct calls to isplit for str and bytes.
Requires lsst/utils#100
Checklist
doc/changes