Review for DM-3811(HSC backport: Include documentation strings for config parameters when they are dumped) #2
Conversation
|
I'm surprised about "Add unnitest for None as docstring", because I don't think we want to allow empty docs. Ever. I think that instead, we should |
| @@ -44,6 +44,7 @@ class Simple(pexConfig.Config): | |||
| d = pexConfig.DictField("dict test", str, str, default={"key":"value"}, | |||
| itemCheck=lambda x: x.startswith('v')) | |||
| n = pexConfig.Field("nan test", float, default=float("NAN")) | |||
| m = pexConfig.Field(None, int, optional=True) | |||
There was a problem hiding this comment.
The HSC commit you're cherry-picking this from is described as "allow None as documentation string". Is that actually something we want? It doesn't sound like a very good idea.
|
@PaulPrice & @jdswinbank, |
|
The correct solution is to enforce doc strings to never be None (that's probably in pipe_base), but until that's propagated through everywhere this was necessary defensive programming -- I found cases that the code failed on without this check. |
laurenam
force-pushed
the
tickets/DM-3811
branch
from
902fccb
to
3b3c7e8
Compare
Following the discussion on DM-3811, the consensus was that having the doc strings print as true comments (i.e. first character in line is '#') is the best option.
laurenam
force-pushed
the
tickets/DM-3811
branch
from
3b3c7e8
to
da6fc20
Compare
No description provided.