Fix Version/s: None
Sprint:DRP S17-2, DRP S17-3, DRP S17-4
Team:Data Release Production
Update DMTN-024 with conventions acquired during pybind11 development.
These guidelines will then need to be RFC'd before being made an official part of the DM developer guide.
|Field||Original Value||New Value|
|Sprint||DRP S17-2 [ 356 ]|
|Team||Data Release Production [ 10301 ]|
|Reviewers||Jim Bosch [ jbosch ]|
|Status||To Do [ 10001 ]||In Review [ 10004 ]|
Pim Schellart [X], I think I've made all the changes we agreed up on yesterday, but sadly I didn't write them down at the time so I might have forgotten something. Changes include:
- Removing the numeric suffix from the filename conventions.
- Mandate use of the "py" namespace alias (at least in source files; open question on whether to do it in headers, I think).
- Rewrote the type deduction guideline.
- New guideline on adding kwargs.
- Permission for letting wrapper module docstrings be empty.
Please take a look when you get a chance, and let me know if you can think of anything else we should have (or if there's anything you disagree with).
I did some minor cleanup. Mostly standardising on SHALL and MAY, given our strange conventions where SHALL and MUST (and SHOULD and MAY) mean the same.
I also added some more examples.
Still to do is adding references and numbering (which I strongly suggest we should do per section to avoid the problem we have with the other guides where adding one rule necessitates renumbering everything).
Going from the header of the document (which I think you copied from the other style guides), think we're supposed to use MUST, SHOULD, and MAY, and those all mean different things (SHOULD is a recommendation, MAY means it's permitted). SHALL means the same as MUST and should be replaced by it.
Yes, that is what I would expect, but the summary of RFC-2119 (at https://developer.lsst.io/coding/intro.html#style-guide-rfc-2119 ) confusingly seems to suggest that SHOULD==MAY and I distinctly remember John Swinbank telling me that in a review.
But I agree that we should make the distinction. As a side note I would suggest we remove that confusing page and directly point to the RFC (or copy over its contents verbatim).
I don't remember that review (don't deny it happened, though), but I do think that you have to follow our documented conventions when writing documentation, even if you don't like them.
That said, I agree that the summary in the Developer Guide looks wrong: it makes no sense to me (and it's certainly not in line with RFC-2119) to suggest that “may” and “should” are synonymous. Perhaps Jonathan Sick can clarify?
Huh, I guess I've been reading it incorrectly the whole time. And I suppose if we're talking about how strict a rule is, the degree to which it's recommended is irrelevant if it's ultimately up to the developer. That said, if we're allowed to use both SHOULD and MAY, would you mind leaving those distinctions in as they were? It won't change how binding the rules are, but I think it does convey the right amount of "recommendation" - I tend to use SHOULD for rules that have exceptions, but only rarely. And of course, if you disagree with me on how strongly we should recommend something in particular, please push back.
Yes, I agree with that. Lets try to use MUST, SHOULD and MAY in the spirit of RFC-2119 even though the latter may or may not be distinct in stringency currently.
Interesting. I can say that https://developer.lsst.io/coding/intro.html#stringency-levels was copied verbatim from the original DM coding standards in confluence (along with our original C++ and Python style guides). I'm very much in favor of fixing that section so that it truly matches RFC-2119 and avoids this confusion.
Before doing so we'd want to ensure that the sprit of our coding standards are still true (we may need to adjust stringency levels in the existing coding standards when we use the correct RFC-2119 terminology). Sounds like this will need a new ticket and Kian-Tat Lim should be brought in the loop as the owner of the coding standards.
If we are going to update the stringency standards I would suggest also updating the rule numbering at the same time. Because the numbering scheme now spans sections it is impossible to add a rule to a section without changing all numbers after it.
And it might make most sense to do this after
DM-8713 is merged (since that removes many rules in the layout section of the C++ style guide).
Much as I'd like to get both of those issues fixed, I worry linking them will mean fixing neither (in the short term, at least). I'd also like to press forward with this ticket without waiting for the more general guidelines to be updated. I think what we've written is not inconsistent with either version of the general guidelines.
Given that the RFC is already looking at this ticket branch, I'd like to just keep this ticket in review until the RFC is complete as use it as the implementation ticket for the RFC (it'll both trigger and be triggered by the RFC - I hope that doesn't bother anyone too much).
|Sprint||DRP S17-2 [ 356 ]||DRP S17-2, DRP S17-3 [ 356, 360 ]|
Jim Bosch, the rule about __truediv__ vs. __div__ should also apply to __itruediv__ and __idiv__. Sorry for not saying so explicitly.
Looks good. One requested change:
Must we change from pybind11 to python in our convention for header files and namespaces that provide shared pybind11 support https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id13 ? I find pybind11 clearer since it is specific to our tool, and all our existing code uses it.
(I'll also say that I wish we used "wrapFoo" instead of "declareFoo" but our existing code goes the other way, so it's probably not worth changing).
Suggested enhancements to the doc:
- Please provide an example for using pybind11::module::import() in https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id12
- Typo "accross"
- In https://developer.lsst.io/v/DM-8577/coding/pybind11_style_guide.html#id16 (use "py::") it says "This alias MUST NOT be defined at namespace scope in header files (see C++ rule 4-13), though it MAY be defined locally within functions in headers.". I don't understand. A few more words or an example would help.
- It is probably worth talking about wrapping enums that are used as ints in the C++ code: we must wrap these as attributes, not enums, in order to avoid pointless casting.
The reason we proposed switching from the pybind11 to python is that pybind11 conflicts with the main pybind11 namespace - even if it's aliased to py, it's still available as pybind11. And while e.g. lsst::afw::geom::pybind11::Foo will be selected in preference to ::pybind11::Foo if you just say pybind11::Foo within lsst::afw::geom, I think it's still confusing.
I don't have strong feelings about wrapFoo vs. declareFoo. If there's broad consensus, I don't think it'd be hard to change, though.
I've added the requested examples, added a rule about when to wrap enums as integer attributes (an excellent suggestion), and clarified that the division rule applies to in-place operators.
`declare` came from swig macros. I don't care. Easy to change. But not really worth it probably.
You make a good point about the pybind11 namespace.
Regarding renaming declareFoo to wrapFoo: how do other team members feel? I would like to switch if folks are comfortable with it, but don't want to get bogged down arguing about minutiae.
|Resolution||Done [ 10000 ]|
|Status||In Review [ 10004 ]||Done [ 10002 ]|
|Resolution||Done [ 10000 ]|
|Status||Done [ 10002 ]||In Review [ 10004 ]|
Russell Owen wrap sounds slightly off to my ear, but I don't have a strong preference.
|Summary||Update pybind11 coding conventions in preparation for RFC||Add pybind11 rules to DM dev-guide|
|Sprint||DRP S17-2, DRP S17-3 [ 356, 360 ]||DRP S17-2, DRP S17-3, DRP S17-4 [ 356, 360, 363 ]|
What's there now looks fine to me, modulo some minor tweaks in the PR. I trust that any changes Jim will ask for are equally fine (or will fix up afterwards if not).
Overlapping reviews with
DM-9098, but all changes have now been implemented.
Will be merged together with pybind11 switch.
|Status||In Review [ 10004 ]||Reviewed [ 10101 ]|
|Resolution||Done [ 10000 ]|
|Status||Reviewed [ 10101 ]||Done [ 10002 ]|
I migrated DMTN-024 to the DM Pybind11 Style Guide, incorporating some changes and lessons learned.