DM-4037: Require non-empty doc string for config parameters #100
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 85.18% 85.24% +0.06%
==========================================
Files 46 46
Lines 3612 3627 +15
==========================================
+ Hits 3077 3092 +15
Misses 535 535
☔ View full report in Codecov by Sentry. |
| self.assertRaises(ValueError, pexConfig.RangeField, "doc", int, default=val, min=val, max=val - 1) | ||
| self.assertRaises( | ||
| ValueError, pexConfig.RangeField, "doc", float, default=val, min=val, max=val - 1e-15 | ||
| ) |
There was a problem hiding this comment.
If I have to be picky, I'd say this should be in its own commit because these two lines would pass regardless of your main change.
There was a problem hiding this comment.
What would be a good commit message for it?
There was a problem hiding this comment.
"Add missing docstrings for configs in unit tests" may be? It'd make sense to include just the new code changes in python/lsst/pex/config/config.py and the new unit test you added to be one commit ("Require non-empty docstring for config parameters"), and everything else in "Add missing docstrings for configs in unit tests".
There was a problem hiding this comment.
I know this is pre-existing style, but the current best practice for this assert is to do something like:
with self.assertRaises(ValueError):
pexConfig.RangeField("doc", float, default=val, min=val, max=val - 1e-15)Since that clearly separates the exception catching code from the code that should generate the error (and in context manager you can do extra things like catch the exception you do get or add extra lines to the block.
There was a problem hiding this comment.
I implemented the current best practice for this assert using separate with blocks in my testDocstring() method but left the pre-existing occurrences untouched since there were too many. They probably need their own ticket.
There was a problem hiding this comment.
"Add missing docstrings for configs in unit tests" may be? It'd make sense to include just the new code changes in
python/lsst/pex/config/config.pyand the new unit test you added to be one commit ("Require non-empty docstring for config parameters"), and everything else in "Add missing docstrings for configs in unit tests".
Done!
enourbakhsh
force-pushed
the
tickets/DM-4037
branch
5 times, most recently
from
0656019
to
bbf5451
Compare
enourbakhsh
force-pushed
the
tickets/DM-4037
branch
from
bbf5451
to
6ed88f0
Compare
There was a problem hiding this comment.
For this package, requiring doc to be non-empty should be considered a user-visible change. I think it should belong in the misc category. You should add a file as described in doc/changes and a 1-line description that all config documentations need to be provided.
Checklist
doc/changes