Add lineage_names to commands
#1148
Conversation
|
Coverage is fine. All of the code that I added got covered. |
| lineage_names = [] | ||
| for cmd in self.lineage: | ||
| lineage_names.append(cmd.name) | ||
| return lineage_names |
There was a problem hiding this comment.
This is a good candidate for a list comprehension:
@property
def lineage_names(self):
# Represents the lineage of a command in terms of command ``name``
return [cmd.name for cmd in self.lineage]
|
This does look like it could be useful, but it would be nice to see how it simplifies the code before getting merged in. Right now it's just adding something that isn't yet used. Even just linking to a WIP branch which uses this should be fine. |
|
Yeah I will be using this in future pull requests. I will note it when I do send the pull request. This just seemed something as a good candidate for a separate PR from what I am working on. |
| @@ -773,6 +781,9 @@ def test_lineage(self): | |||
| self.cmd.lineage = ['foo'] | |||
| self.assertEqual(self.cmd.lineage, ['foo']) | |||
|
|
|||
| def test_lineage_names(self): | |||
There was a problem hiding this comment.
This isn't technically wrong, but it's a little confusing given the test above has self.assertEqual(self.cmd.lineage, ['foo']). Do you think it's worth changing the test_lineage test to set the lineage to an object to be consistent with real world usage?
There was a problem hiding this comment.
Yeah I can update the test.
|
Given the link to #1151, I think this seems reasonable. Just had one small question about the tests but otherwise, |
It is a convenience attribute to get the names of all of the commands in a lineage without having to iterate through the lineage each time.
kyleknap
force-pushed
the
lineage-names
branch
from
2cb30b3 to
9e40bfb
Compare
|
Coveralls is not correct again. I switched those test to |
Add ``lineage_names`` to commands
It is a convenience attribute to get the names of all of the commands in a lineage without having to iterate through the lineage each time. This saves us from a lot of duplicate code.
So for a
lineageattribute, you get the elements in the list in terms of command objects. For thelineage_namesattribute, you get the elements in the list in terms of the names of command objects. This attribute I could see really useful for at least two scenarios:Generating event names. All you would have to do to get the command's representation in the event name is
'.'.join(cmd.lineage_names)Looking for related command tags in the
TagTopicDB. All of therelated commandtag values follow the format of<service> [operation] .... To look for these values, all you would have to do is' '.join(cmd.lineage_names).I also considered making it a utils function, but it would be pretty annoying to have to import it each time you need to use it. Let me know what you think.
cc @jamesls @danielgtaylor