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

General cleanup of Events package

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      35
    • Sprint:
      W15 Dec Sprint 1, W15 Dec Sprint 2, W15 Dec Sprint 3, W15 Jan Sprint 1, W15 Jan Sprint 2
    • Team:
      Data Facility

      Description

      There are some obsolete classes and code in the ctrl_events package that needs to be removed and/or updated. PipelineLogEvent, for example. That not only is no longer used, but it is applications specific, and should have been part of another package in the first place, subclassed from this package.

        Attachments

          Issue Links

            Activity

            Hide
            spietrowicz Steve Pietrowicz added a comment -

            The consolidated commit changes for this are done and this is now ready to be reviewed.

            Show
            spietrowicz Steve Pietrowicz added a comment - The consolidated commit changes for this are done and this is now ready to be reviewed.
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            Paul Price Would you still be available to review this?

            Show
            spietrowicz Steve Pietrowicz added a comment - Paul Price Would you still be available to review this?
            Hide
            price Paul Price added a comment -

            Sorry I haven't done this yet. Thank you for the prod. I'll do it today.

            Show
            price Paul Price added a comment - Sorry I haven't done this yet. Thank you for the prod. I'll do it today.
            Hide
            price Paul Price added a comment -

            I've made several comments on github (https://github.com/lsst/ctrl_events/pull/1). The major ones are essentially:

            • Commits aren't orthogonal, e.g., copyrights are modified in different commits; some commits contain changes to both copyrights and functional code. Give each commit a short one-line summary, then leave a blank line and then explain the commit in more detail (e.g., motivation, solution, other options) with a ~72 char max line length. See https://confluence.lsstcorp.org/display/LDMDG/Git+Commit+Best+Practices
            • Documentation is very lacking. Each function and variable declared in a header should have documentation unless its function is completely obvious from the declaration. Some of the documentation is in the implementation files instead of the header, but even then it's sparse.
            • Passing by raw pointer or value instead of reference or const-reference.
            • "const" is not common, and where it is used it's used before the type instead of after. Const is your friend — it keeps you from doing something bad, so use it as much as you can.
            • Line lengths violate the limit of 110 chars.
            • Naming is unhelpful, e.g., naming after a type rather than function (ps, psp).
            • Use the container's iterators (it's simple with auto) instead of iterating over the container's length.
            • Not using braces on two-line statements.
            Show
            price Paul Price added a comment - I've made several comments on github ( https://github.com/lsst/ctrl_events/pull/1 ). The major ones are essentially: Commits aren't orthogonal, e.g., copyrights are modified in different commits; some commits contain changes to both copyrights and functional code. Give each commit a short one-line summary, then leave a blank line and then explain the commit in more detail (e.g., motivation, solution, other options) with a ~72 char max line length. See https://confluence.lsstcorp.org/display/LDMDG/Git+Commit+Best+Practices Documentation is very lacking. Each function and variable declared in a header should have documentation unless its function is completely obvious from the declaration. Some of the documentation is in the implementation files instead of the header, but even then it's sparse. Passing by raw pointer or value instead of reference or const-reference. "const" is not common, and where it is used it's used before the type instead of after. Const is your friend — it keeps you from doing something bad, so use it as much as you can. Line lengths violate the limit of 110 chars. Naming is unhelpful, e.g., naming after a type rather than function ( ps , psp ). Use the container's iterators (it's simple with auto ) instead of iterating over the container's length. Not using braces on two-line statements.
            Hide
            spietrowicz Steve Pietrowicz added a comment - - edited

            Thanks for looking at this. The C++ things you've pointed out are purely from my lack of experience with C++ and I'll look to get these corrected. The documentation should all be in the C++ code, which I believe at the time this was originally written was considered "OK" to do, but I can get that moved over to the headers.

            Show
            spietrowicz Steve Pietrowicz added a comment - - edited Thanks for looking at this. The C++ things you've pointed out are purely from my lack of experience with C++ and I'll look to get these corrected. The documentation should all be in the C++ code, which I believe at the time this was originally written was considered "OK" to do, but I can get that moved over to the headers.

              People

              • Assignee:
                spietrowicz Steve Pietrowicz
                Reporter:
                spietrowicz Steve Pietrowicz
                Reviewers:
                Paul Price
                Watchers:
                Kian-Tat Lim, Paul Price, Russell Owen, Steve Pietrowicz
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel