Mention other platforms in 'pytest/pytest-cov not found' warning #337
Conversation
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
| "The Python module 'pytest' was not found, pytests cannot be run. " | ||
| "On Ubuntu/Debian, install the 'python3-pytest' package. " | ||
| "On other platforms, install 'pytest' using pip.") |
There was a problem hiding this comment.
I wonder if it makes more sense to just point to the generic installation docs instead. That is, something like:
"The Python module 'pytest' was not found, pytests cannot be run. "
"See https://docs.ros.org/en/rolling/Installation.html to install pytest for your platform.")
This has the downside that we are embedding a rolling URL in the code, but it avoids duplicating the install instructions. Thoughts?
There was a problem hiding this comment.
it would make sense for pytest because it is part of a normal ROS 2 installation (however, pytest-cov isn't), but besides embedding rolling in the URL, I kind of prefer the current warning messages because they tell you exactly what you need to do (install ___ package) instead of pointing you to the generic installation docs just to install a Python package or two.
In any case, I can switch to that for pytest if you prefer.
There was a problem hiding this comment.
another thing: should ament really link to ROS 2 like that? As opposed to the other way around, ROS 2 linking to ament.
There was a problem hiding this comment.
I kind of prefer the current warning messages because they tell you exactly what you need to do (install ___ package) instead of pointing you to the generic installation docs just to install a Python package or two.
Yeah, that's true.
another thing: should
amentreally link to ROS 2 like that? As opposed to the other way around, ROS 2 linking toament.
In theory, no. In practice, it seems unlikely that anyone outside the ROS 2 ecosystem is using ament (at least, I don't know of any). It would be nice to be proven wrong, though :).
Overall, I agree with you about keeping the current message. The one thing I would say is let's change the message to say "On Linux, install the 'python3-pytest' package", since that will also cover RHEL/Fedora.
There was a problem hiding this comment.
In theory, no. In practice, it seems unlikely that anyone outside the ROS 2 ecosystem is using ament (at least, I don't know of any). It would be nice to be proven wrong, though :).
maybe one day! 😄
Overall, I agree with you about keeping the current message. The one thing I would say is let's change the message to say "On Linux, install the 'python3-pytest' package", since that will also cover RHEL/Fedora.
done: 0752f31
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
|
CI (just to ensure that we don't have tests looking for an exact string):
|
|
Looks good, going ahead and merging. Thanks for the contribution! |
christophebedard
deleted the
mention-other-platforms-in-pytest-pytest-cov-message
branch
Relates to #274
Signed-off-by: Christophe Bedard bedard.christophe@gmail.com