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

Please include Git commit message guidelines in "setup and best practices" section of developer guide

    Details

      Description

      We currently provide some guidelines on "best practices" for commit messages in the development workflow documentation.

      However, we also provide a document on "Git Setup and Best Practices", which makes no reference to the above guidelines.

      It would be helpful to ensure that all our guidelines for git usage are available from the same location.

      (It would be even better if we could be clear about what's a requirement and what's a suggestion...)

        Attachments

          Issue Links

            Activity

            Hide
            jsick Jonathan Sick added a comment -

            I’ve merge the originally-requested change. I’ve moved the second request into DM-5842 and will follow-up there.

            Show
            jsick Jonathan Sick added a comment - I’ve merge the originally-requested change. I’ve moved the second request into DM-5842 and will follow-up there.
            Hide
            swinbank John Swinbank added a comment -

            I think I'm not asking for such a big change as you are worried about.

            At the moment, we simply have a statement that "we follow" some particular set of guidelines. My concern is simply that we don't, or at least we're not required to. Softening the language to "we suggest", or similar, instead would resolve the issue as far as I'm concerned.

            Show
            swinbank John Swinbank added a comment - I think I'm not asking for such a big change as you are worried about. At the moment, we simply have a statement that "we follow" some particular set of guidelines. My concern is simply that we don't, or at least we're not required to. Softening the language to "we suggest", or similar, instead would resolve the issue as far as I'm concerned.
            Hide
            jsick Jonathan Sick added a comment -

            John Swinbank The original work on making the workflow guide was in DM-4412, and see discussion in https://github.com/lsst-dm/dm_dev_guide/pull/4

            I think there are two options to solve the “best practices in a workflow policy document:”

            1. I put a notice in each best practice appendix section saying “this is not a requirement.” Although to be honest, I think the 50/72 rule for commit messages should be treated as a requirement if you’re a Git user. We have no requirement on commit message content, although I believe it’s fair game for a reviewer to ask for commit messages to be improved in the spirit of the appendix. Similarly for commit organization; it may not be a requirement, but I think a reviewer can ask for commits to be squashed or split in the spirit of that appendix.
            2. I remove the commit message/organization appendices from the workflow document and put them in a new page in the tools section (underneath the pages on configuring Git and Git LFS).

            Any opinions? Thoughts Kian-Tat Lim?

            Show
            jsick Jonathan Sick added a comment - John Swinbank The original work on making the workflow guide was in DM-4412 , and see discussion in https://github.com/lsst-dm/dm_dev_guide/pull/4 I think there are two options to solve the “best practices in a workflow policy document:” I put a notice in each best practice appendix section saying “this is not a requirement.” Although to be honest, I think the 50/72 rule for commit messages should be treated as a requirement if you’re a Git user. We have no requirement on commit message content, although I believe it’s fair game for a reviewer to ask for commit messages to be improved in the spirit of the appendix. Similarly for commit organization; it may not be a requirement, but I think a reviewer can ask for commits to be squashed or split in the spirit of that appendix. I remove the commit message/organization appendices from the workflow document and put them in a new page in the tools section (underneath the pages on configuring Git and Git LFS). Any opinions? Thoughts Kian-Tat Lim ?
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Looks great! Thank you for the quick change!!!

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Looks great! Thank you for the quick change!!!
            Hide
            swinbank John Swinbank added a comment - - edited

            Thanks, Jonathan Sick.

            To expand a little on the second point in the issue (which, for clarity, I added, not Vishal Kasliwal [X]): I am not asking for a change in policy here, merely a clarification of the documentation. If this particular practice is not a requirement then the statement that "we follow" it seems pretty misleading, regardless of whether you or I happen to think it would be a good idea.

            Of course, maybe nobody actually knows what the policy is, and hence the lack of clarity in the document.

            (And, I grant you, it's orthogonal to the main thrust of this ticket.)

            Show
            swinbank John Swinbank added a comment - - edited Thanks, Jonathan Sick . To expand a little on the second point in the issue (which, for clarity, I added, not Vishal Kasliwal [X] ): I am not asking for a change in policy here, merely a clarification of the documentation. If this particular practice is not a requirement then the statement that "we follow" it seems pretty misleading, regardless of whether you or I happen to think it would be a good idea. Of course, maybe nobody actually knows what the policy is, and hence the lack of clarity in the document. (And, I grant you, it's orthogonal to the main thrust of this ticket.)
            Hide
            jsick Jonathan Sick added a comment -

            Does this solve your issue?

            Show
            jsick Jonathan Sick added a comment - Does this solve your issue?
            Hide
            jsick Jonathan Sick added a comment -

            Regarding #2, I believe that was purposefully put in an appendix because it wasn’t a requirement. However, any professional software developer should already strive to make useful commit messages. The 50/72 character rules are extremely strong conventions in Git.

            Show
            jsick Jonathan Sick added a comment - Regarding #2, I believe that was purposefully put in an appendix because it wasn’t a requirement. However, any professional software developer should already strive to make useful commit messages. The 50/72 character rules are extremely strong conventions in Git.
            Hide
            jsick Jonathan Sick added a comment -

            So there are two issues.

            1. The Git Setup and Best Practices does link to the commit message guidelines which belong to our workflow process document however it’s not clear that that’s where you’d go. I think the correct solution here is to rename Git Setup and Best Practices to Git Configuration Best Practices and make it even more clear in the See also section that a person should consult the Workflow guide for details on our usage of Git.

            2. Your request "It would be even better if we could be clear about what's a requirement and what's a suggestion” should be filed in a separate ticket and will require consultation with whatever group governs policy (TCT?).

            Show
            jsick Jonathan Sick added a comment - So there are two issues. 1. The Git Setup and Best Practices does link to the commit message guidelines which belong to our workflow process document however it’s not clear that that’s where you’d go. I think the correct solution here is to rename Git Setup and Best Practices to Git Configuration Best Practices and make it even more clear in the See also section that a person should consult the Workflow guide for details on our usage of Git. 2. Your request "It would be even better if we could be clear about what's a requirement and what's a suggestion” should be filed in a separate ticket and will require consultation with whatever group governs policy (TCT?).

              People

              • Assignee:
                jsick Jonathan Sick
                Reporter:
                vpk24 Vishal Kasliwal [X] (Inactive)
                Reviewers:
                Vishal Kasliwal [X] (Inactive)
                Watchers:
                John Swinbank, Jonathan Sick, Vishal Kasliwal [X] (Inactive)
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel