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-14391 Simplify initialization of display_firefly and firefly_client #11
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.
This looks fine, generally. I'm not familiar with FireflyClient
so I have to take your word on those bits.
@@ -0,0 +1,273 @@ | |||
.. _display_firefly: |
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 think doing a git mv
would have let you preserve the diff.
_fireflyClient = firefly_client.FireflyClient(channel=name, **kwargs) | ||
else: | ||
_fireflyClient = firefly_client.FireflyClient(url, | ||
channel=name, **kwargs) |
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.
According to our standards, I think channel
should be aligned with url
.
_fireflyClient = firefly_client.FireflyClient(channel=name, **kwargs) | ||
else: | ||
_fireflyClient = firefly_client.FireflyClient(url, | ||
channel=name, **kwargs) | ||
except Exception as e: |
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.
Typically it's better to trap the exceptions you expect. Trapping a bare exception means you can't tell what actually went wrong when something unexpected happens.
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.
Agreed. I'll trap on the two exceptions that are raised, one for a nonsense url and one for a working web server that is not a Firefly url.
Changes to the
display_firefly
backend to enable easier initialization. These changes go together with the 2.0.0+ version offirefly_client
.name
to None, to allowfirefly_client
to auto-initialize the channel.host
tourl
to track the new initialization offirefly_client.FireflyClient
.url
unspecified, to allowFireflyClient
to initialize from environment variables, as will be provided in LSST Science Platform environments.FireflyClient
instance and the browser url as attributes.