Code review comment for ~artivis/lpci:feature/ros-plugins

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

Jeremie, thanks for the merge proposal.

CI currently fails as there are two uncovered lines, see buildlog below (below `unmerged commits`):

```
:: lpcraft/plugins/plugins.py 284 2 64 1 99% 482, 491
```

Could you please fix these?

As lpcraft is both the CI runner for Launchpad and a standalone tool, I'd like to know how you are planning to use it. Do you use or plan to use it in Launchpad CI?

I am asking as this would influence the plugin story. For a standalone lpcraft we could easily add setuptools based plugins ( https://pluggy.readthedocs.io/en/stable/#loading-setuptools-entry-points ), so we could have the plugin in a standalone plugin package. For Launchpad CI this is more elaborate, and currently out of scope.

I am discussing these options, as at least for me ROS is a business domain I have never touched before, and including these plugins means I (we) have to maintain them.

Fortunately, the code looks pretty straightforward. I will discuss this MP at today's standup and will get back to you.

Afterwards, I will also give you a detailed review. At very least you would need to add some descriptive docstrings which explain the used business terminology.

Also, does this MP depend on https://code.launchpad.net/~artivis/lpcraft/+git/lpcraft/+merge/435370 or would merging this MP on its own already be helpful?

« Back to merge proposal