Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DM-40156: Some code cleanups #869

Merged
merged 31 commits into from
Jul 24, 2023
Merged

DM-40156: Some code cleanups #869

merged 31 commits into from
Jul 24, 2023

Conversation

timj
Copy link
Member

@timj timj commented Jul 21, 2023 •
edited
Loading

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@codecov
Copy link

codecov bot commented Jul 21, 2023 •
edited
Loading

Codecov Report

Patch coverage: 83.61% and project coverage change: +0.03% 🎉

Comparison is base (ee6d72d) 87.76% compared to head (3c84fee) 87.79%.

❗ Current head 3c84fee differs from pull request most recent head 05bb4df. Consider uploading reports for the commit 05bb4df to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
+ Coverage   87.76%   87.79%   +0.03%     
==========================================
  Files         274      274              
  Lines       36057    36005      -52     
  Branches     7562     7535      -27     
==========================================
- Hits        31645    31612      -33     
+ Misses       3241     3227      -14     
+ Partials     1171     1166       -5
Files Changed Coverage Δ
python/lsst/daf/butler/cli/cmd/commands.py 88.92% <0.00%> (-0.04%) ⬇️
python/lsst/daf/butler/core/datasets/type.py 84.25% <0.00%> (ø)
python/lsst/daf/butler/core/dimensions/_config.py 84.81% <0.00%> (ø)
...ython/lsst/daf/butler/core/storageClassDelegate.py 54.54% <ø> (ø)
python/lsst/daf/butler/registries/remote.py 80.40% <ø> (-0.20%) ⬇️
...ython/lsst/daf/butler/registry/_butler_registry.py 87.09% <0.00%> (ø)
...thon/lsst/daf/butler/registry/_registry_factory.py 84.00% <0.00%> (ø)
python/lsst/daf/butler/registry/managers.py 90.54% <0.00%> (ø)
...thon/lsst/daf/butler/script/certifyCalibrations.py 37.50% <0.00%> (+2.20%) ⬆️
python/lsst/daf/butler/script/exportCalibs.py 19.29% <0.00%> (ø)
... and 59 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timj timj requested a review from andy-slac July 24, 2023 14:57
timj added 26 commits July 24, 2023 07:57
@timj
Add missing type annotation for butler test
401ac97
@timj
Use property access rather than getattr is name is known
bc3fc08
@timj
Use modern isinstance syntax with A | B
1a89cb6
@timj
Use AbstractContextManager instead of typing.ContextManager
9682c12
@timj
Appease bugbear B023 by binding the loop parameters
53790c7 
This does not affect the tests because these functions are
used immediately inside the loop.
@timj
Use explicit exception in test rather than generic Exception
9e85579
@timj
Use contextlib.suppress
0dead7f
@timj
Use dict.get() rather than "in" check
de90c3c
@timj
Use "a in b" instead of "a in b.keys()"
7279cf8
@timj
Invert ternary logic
aaf37a2 
a = x if x else y

instead of

a = y if not x else x
@timj
Invert logic in if statement to avoid NOT
fe4fda0
@timj
Use bool(x) rather than if x return True
3aea3ec
@timj
Simplify without_datastore assignment
3531ab6
@timj
Simplify a for loop by using all()
266519d
@timj
Combine two if branches that return the same thing
ad214ac
@timj
Combine nested ifs into single if
f1117af
@timj
Replace some nested with statements
df781a5 
Keep ones involving assertRaises and assertWarns since then
it is easier to see the separation between the test code and
the code being tested.
@timj
Change warnings.warn to a print for the command-line user
fa22b48 
warnings.warn has too much other information that is irrelevant
to the user of the command-line tool.
@timj
Add explicit stacklevel to warnings
59fe2e9 
The test code doesn't really need it but it doesn't hurt and
it makes it easier to find warnings in the future that lack
the stacklevel.
@timj
Do not use enumerate if counter is not being used
2fd8cbc
@timj
Use _ for loop variables that are known to be ignored
c935f7a
@timj
Use .values when key is not needed in loop
474b9fb
@timj
Raise AssertionError rather than assert False
579ac77 
assert can be removed as a no-op depending on how python is run.
@timj
Use loop variable in test
2f89f27
@timj
Clarify the purpose of the loop variable
e12e27e
@timj
Do not overwrite variable of thing being iterated in loop
448ffde
timj added 2 commits July 24, 2023 07:57
@timj
Add from when raising exception inside other exception handler
2ea7bba
@timj
Remove lru_cache on method
a5d79c4 
lru_cache on methods doesn't work properly. Furthermore if we
need to cache the collection type we should be caching it
elsewhere inside registry.
andy-slac
andy-slac approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple of minor comments.

python/lsst/daf/butler/core/ddl.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/core/dimensions/_coordinate.py Show resolved Hide resolved
python/lsst/daf/butler/registry/databases/sqlite.py Show resolved Hide resolved
python/lsst/daf/butler/script/transferDatasets.py Outdated Show resolved Hide resolved
@timj timj merged commit 0787220 into main Jul 24, 2023
14 checks passed
@timj timj deleted the tickets/DM-40156 branch July 24, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-slac andy-slac andy-slac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timj @andy-slac