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

General cleanup of Events package

    XMLWordPrintable

    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:

                  CI Builds

                  No builds found.