DM-41062: Add topic finding to efdUtils #64
Conversation
| @@ -553,3 +553,29 @@ def getSubTopics(client, topic): | |||
| loop = asyncio.get_event_loop() | |||
| topics = loop.run_until_complete(client.get_topics()) | |||
| return sorted([t for t in topics if t.startswith(topic)]) | |||
|
|
|||
There was a problem hiding this comment.
Now that with this new findTopic function, do you still want the getSubTopics function above?
If yes, do you consider changing their names? Just by their names it's confusing what is for what. Might be even better just to have one function taking a regular expression.
There was a problem hiding this comment.
So I do think that we still need both - here is how I imagine using them:
There was a problem hiding this comment.
And I thought that the naming made the usage pretty clear, but maybe I am wrong. I don't like the idea of the regex, mainly because I wrote these to be easy, and as a self-declared regex-phobe, not only would I not be confident in how to best write that, I wouldn't even know how to use my own tools! 😄
There was a problem hiding this comment.
Given that, what do you think is best here?
There was a problem hiding this comment.
Naively I would be tempted to use a utility this way:
getTopics(client, "*airflow*") # for finding whatever containing the airflow
getTopics(client, "lsst.sal.ESS.*") # to see what lives inside ESS
But I'm not a user of this and won't be in the short term, so I'd leave the judgement to you.
There was a problem hiding this comment.
(Actually that wasn't really regexp.. But you get my idea on wildcarding)
There was a problem hiding this comment.
Those exact examples now work 🙂
|
(silly me... Because I opened the PR I can't approve my own PR... You can consider this approved) |
mfisherlevine
force-pushed
the
tickets/DM-41062
branch
5 times, most recently
from
b999f75
to
f300141
Compare
mfisherlevine
force-pushed
the
tickets/DM-41062
branch
from
f300141
to
fcdad76
Compare
mfisherlevine
force-pushed
the
tickets/DM-41062
branch
from
fcdad76
to
ef8ab02
Compare
tests/test_butlerUtils.py
Outdated
| @@ -110,7 +111,7 @@ def setUp(self): | |||
| self.assertIsInstance(self.dataCoordMinimal, dafButler.dimensions.DataCoordinate) | |||
| # NB the type check below is currently using a non-public API, but | |||
| # at present there isn't a good alternative | |||
| viewType = dafButler.core.dimensions._coordinate._DataCoordinateFullView | |||
| viewType = dafButler.dimensions._coordinate._DataCoordinateFullView | |||
There was a problem hiding this comment.
Better to drop this assertIsInstance check or replace it with a check on something that is public, like lsst.daf.butler.NamedKeyMapping. Leaving this dependency on a private symbol is just asking for your code to break unexpectedly in the future, and it's just a test, not a production use case.
Opening the PR so it's easier to review