Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ctrl_mpexec, drp_tasks, ip_isr, pex_config, pipe_base, pipe_tasks, verify
-
Story Points:10
-
Epic Link:
-
Sprint:Ops Pipelines 2023
-
Team:Data Release Production
Description
Currently, doc=None is allowed for config parameters. We should not be allowing for configuration parameters with no description, so the existence of a doc string should be imposed.
Attachments
Activity
Robert Lupton No, I don't think we will have any failures as the doc is cast to a str before prepending the true comment style (# as 1st character in line). I think the only other place this was causing a problem was introduced as a check for triple quotes in the doc string (as the presence of them within the docstring itself would have resulted in a delimiter collision with the use of the ''' as delimiting the multiline string). The fix in HSC-1072 was just to add an if doc: prior to the test. Since we are now using the true comment style, there is no such restriction for the docstring itself, so the check is never made.
I have tested this by adding a doc=None test config parameter in the tests/config.py unittest in pex_config and it passed through just fine. Note that this test did not end up getting added to master given the discussions suggesting that doc=None should NOT be allowed/accommodated, and hence the creation of this ticket!
Do let me know if I am mistaken and there is another mode where this could fail.
OK, if you're sure it's not a problem proceed as planned!
I'm reluctant to give up and say that empty docstrings should be permitted but not recommended. I think that it is a very rare case that the config parameter name is so self-explanatory that nothing additional can be provided by a docstring. While useless docstrings can be caught during code review, having an additional layer of technical avoidance, at minimal cost, can be useful.
Task framework -> team DAX.
The requirement for non-empty docstrings for configuration parameters is now enforced via `pex_config.` Furthermore, I have incorporated a unit test to ensure its functionality. For the Jenkins run to be successful, I had to insert docstrings for all the absent ones in other packages, primarily within their respective unit tests.
Hi Arun, could you kindly review this ticket, please? I had to work with the `pex_config` package, but also ended up fixing empty docstrings in various other packages.
Jenkins is green: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/38958/pipeline
Thanks for incorporating the changes Erfan
I think empty doc strings should be allowed (although not recommended), otherwise we'll just get doc strings such as " " and "doc". Wasn't the problem with None, not ""?
Also, doesn't this need to be merged before we can merge
DM-3811? Otherwise --show config can fail (this was how I first found the problem)