Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5456

Update Python coding standard based on RFC-162

    Details

      Description

      RFC-162 proposes that the Python coding standard should be rewritten in terms of PEP-8. Kian-Tat Lim has requested that the proposed new text of the coding standard be submitted for his approval prior to incorporation into developer.lsst.io, presumably as a pull request. This ticket covers that rewriting.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            I like the new version (N251 vs E251 noted on PR) and I'm happy for it to be merged with one caveat. Re-reading it I'm now more convinced that we have the order in section 1 wrong. I really think it should be:

            1. We use PEP8
            2. These are the exceptions to PEP8
            3. This is how you can check whether you are compliant
            4. This is how you can automatically fix up your code.

            Out of scope for this work, I feel that section 10 "Idiomatic Python" is not named properly. It covers two distinct topics. It starts by giving general programming advice, but it then it describes mandatory coding approaches. I think hiding the "You MUST do THIS" parts of the document in an "Idioms" section is underselling it.

            Show
            tjenness Tim Jenness added a comment - I like the new version (N251 vs E251 noted on PR) and I'm happy for it to be merged with one caveat. Re-reading it I'm now more convinced that we have the order in section 1 wrong. I really think it should be: We use PEP8 These are the exceptions to PEP8 This is how you can check whether you are compliant This is how you can automatically fix up your code. Out of scope for this work, I feel that section 10 "Idiomatic Python" is not named properly. It covers two distinct topics. It starts by giving general programming advice, but it then it describes mandatory coding approaches. I think hiding the "You MUST do THIS" parts of the document in an "Idioms" section is underselling it.
            Hide
            jsick Jonathan Sick added a comment -

            Much thanks for your patient review Tim Jenness. I switched the order of Section 1 around as you suggest and it does work better. Also fixed the E251 error.

            As for the Idiomatic Python section; I’m happy to try to fix that before merging. I think part of the problem may be that some of the sections don’t have SHOULD/MUST -type language in their headers yet because they were guidelines creating from passing mentions in the original style guide. For example "Use ‘as' when catching an exception” could probably be “The ‘as’ syntax MUST be used when catching an exception." Opinions? Or should we just merge?

            Show
            jsick Jonathan Sick added a comment - Much thanks for your patient review Tim Jenness . I switched the order of Section 1 around as you suggest and it does work better. Also fixed the E251 error. As for the Idiomatic Python section; I’m happy to try to fix that before merging. I think part of the problem may be that some of the sections don’t have SHOULD/MUST -type language in their headers yet because they were guidelines creating from passing mentions in the original style guide. For example "Use ‘as' when catching an exception” could probably be “The ‘as’ syntax MUST be used when catching an exception." Opinions? Or should we just merge?
            Hide
            jsick Jonathan Sick added a comment - - edited

            In addition, I’ll repeat here a comment from the PR on likely future tickets on this page:

            1. PEP 8 naming (RFC-97)
            2. `super` being more acceptable
            3. Elimination of non-PEP 8 whitespace rules in https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#keyword-assignment-operators-should-be-surrounded-by-a-space-when-statements-appear-on-multiple-lines
            4. Eliminate mention of test suites in non-test modules: https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#standard-code-order-should-be-followed
            5. Comments, even for private APIs, should be docstrings: https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#docstrings-should-be-written-for-all-public-modules-functions-classes-and-methods
            Show
            jsick Jonathan Sick added a comment - - edited In addition, I’ll repeat here a comment from the PR on likely future tickets on this page: PEP 8 naming ( RFC-97 ) `super` being more acceptable Elimination of non-PEP 8 whitespace rules in https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#keyword-assignment-operators-should-be-surrounded-by-a-space-when-statements-appear-on-multiple-lines Eliminate mention of test suites in non-test modules: https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#standard-code-order-should-be-followed Comments, even for private APIs, should be docstrings: https://developer.lsst.io/v/DM-5456/coding/python_style_guide.html#docstrings-should-be-written-for-all-public-modules-functions-classes-and-methods
            Hide
            Parejkoj John Parejko added a comment -

            One more addition for a possible future ticket is to make this bit match PEP-257 (single line docstrings should be a single line, not end on a new line):

            https://developer.lsst.io/coding/python_style_guide.html#docstrings-should-be-begin-with-and-terminate-with-on-its-own-line

            Show
            Parejkoj John Parejko added a comment - One more addition for a possible future ticket is to make this bit match PEP-257 (single line docstrings should be a single line, not end on a new line): https://developer.lsst.io/coding/python_style_guide.html#docstrings-should-be-begin-with-and-terminate-with-on-its-own-line
            Hide
            tjenness Tim Jenness added a comment -

            Merge what you've got. I probably need to modify the idioms section anyhow when I work on python 3 support.

            Show
            tjenness Tim Jenness added a comment - Merge what you've got. I probably need to modify the idioms section anyhow when I work on python 3 support.

              People

              • Assignee:
                jsick Jonathan Sick
                Reporter:
                tjenness Tim Jenness
                Reviewers:
                Tim Jenness
                Watchers:
                John Parejko, Jonathan Sick, Kian-Tat Lim, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel