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
DM-20941: Cleanups (fix third-party deps in table file; remove dependency on future; update license statements) #48
Conversation
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.
Looks great. Some minor comments on the table file.
ups/verify.table
Outdated
setupRequired(pyyaml) | ||
setupRequired(requests) | ||
setupRequired(numpy) | ||
setupRequired(base) | ||
setupRequired(utils) | ||
setupRequired(pex_exceptions) |
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 could probably remove this dependency by noting that lsst.pex.exceptions.NotFoundError
looks like a LookupError
in Python land.
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.
Can I just switch it without introducing a breaking API change?
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.
There is no API change is there? I think you only use NotFoundError when you are using getPackageDir. Changing those to LookupError should be fine shouldn't it?
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.
ah yes. perfect
ups/verify.table
Outdated
@@ -1,4 +1,8 @@ | |||
setupRequired(sconsUtils) |
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.
We always list the sconsUtils dependency if you a package is using sconsUtils.
ups/verify.table
Outdated
setupRequired(pyyaml) | ||
setupRequired(requests) | ||
setupRequired(numpy) | ||
setupRequired(base) |
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.
utils is bringing in base, and your __init__.py
files go out of their way to work even if base
is missing, so I'd tend to not put base
in here.
All of these features are now mandatory in our supported Python version.
This makes the code fully Python 3 :) Also drops the python_future dependency from the table file.
This preamble matches the standard at https://github.com/lsst/templates/tree/master/file_templates/stack_license_preamble_py
lsst.pex.exceptions.NotFoundError is equivalent to the built-in LookupError.
41bfd5d
to
44ef913
Compare
Thanks @timj I've made those fixes. I'll put you down as the official reviewer in Jira too. |
__future__
importsfuture
andpast
packagespython_future
ups/verify.build
file that not used by anything anymore.Note I tried to remove the empty
__init__.py
files, but did run into test issues. I'll have to circle back on that.CI link: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/30244/pipeline