search

colcon / colcon-ros

Extension for colcon to support ROS packages
http://colcon.readthedocs.io
Apache License 2.0
13 stars 26 forks source link

Add package maintainer information to metadata #139

Closed cottsay closed 2 years ago

cottsay commented 2 years ago

This information could be useful to other extensions.

cottsay commented 2 years ago

Would it make sense to use a set() instead of a list to prevent duplication?

I think that avoiding duplication would be nice, but maintaining order might also be desirable. So I opted for a list and will leave it up to the consumer to de-duplicate, possibly losing the order in the process.

methylDragon commented 2 years ago

Just wanted to double check why the metadata appending is in the Identify step as opposed to the Augment step though :o Maybe I don't know enough about the flow..

cottsay commented 2 years ago

Just wanted to double check why the metadata appending is in the Identify step as opposed to the Augment step though :o Maybe I don't know enough about the flow..

Your intuition here is absolutely right, in my opinion. I believe that the majority of what's currently happening in this identification extension should be moved into one or more augmentation extensions.

From what I can tell, the roles of each have evolved since colcon was created, and more work has shifted from identification to augmentation. Things get really weird when you start using colcon.pkg files to override parts of the default identification pipeline - sometimes the descriptor populates dependencies from the underlying package metadata, and sometimes the use of colcon.pkg overrides everything. The latter will happen here, but the former will happen with a setup.cfg file.

It isn't consistent, but it hasn't bothered me enough to fix it. Yet.