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

Make pipe_analysis flake8 compliant and enable Travis and branch protections

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_analysis
    • Labels:
      None
    • Story Points:
      5
    • Epic Link:
    • Sprint:
      DRP F20-3 (Aug)
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      We have yet to enable a flake8 travis job and branch protections for this repo. As such, our code is becoming very flake8 un-compliant. This ticket is to make all of pipe_analysis flake8 compliant while allowing for the Rubin Style Guide exceptions to PEP 8 , and then turn on the flake8 travis job and branch protections so that no further violations will be able to sneak their way in.

        Attachments

          Issue Links

            Activity

            Hide
            lauren Lauren MacArthur added a comment -

            I can confirm that the Travis check is indeed now firing.  Here is what I got when I pushed an in-progress (i.e. not yet fully PEP8-ified) branch:

             
            Tim Jenness, are there any additional steps that need to be taken to "turn on" branch protection?

            Show
            lauren Lauren MacArthur added a comment - I can confirm that the Travis check is indeed now firing.  Here is what I got when I pushed an in-progress (i.e. not yet fully PEP8-ified) branch:   Tim Jenness , are there any additional steps that need to be taken to "turn on" branch protection?
            Hide
            lauren Lauren MacArthur added a comment -

            Would you have time to give this a look?  As the ticket describes, the main goal here is to get all code in pipe_analysis conforming with PEP8/Rubin standards.  As I was making these fixes, I couldn't help but notice a number of inconsistencies and unrecommended practices largely in the module documentation, so I took the opportunity to do some homogenizing/standardizing.  I tried to keep the independent changes in the separate commits, but it is likely a few of the latter slipped in former. This is also not meant to be a fully thorough upgrade to the documentation, but does fix some of the more egregious precedents that had been set (and, unfortunately, were being subsequently followed). I also certainly do not expect a line-by-line review here, but if you see anything particularly offensive, please let me know!

            Show
            lauren Lauren MacArthur added a comment - Would you have time to give this a look?  As the ticket describes, the main goal here is to get all code in pipe_analysis conforming with PEP8/Rubin standards.  As I was making these fixes, I couldn't help but notice a number of inconsistencies and unrecommended practices largely in the module documentation, so I took the opportunity to do some homogenizing/standardizing.  I tried to keep the independent changes in the separate commits, but it is likely a few of the latter slipped in former. This is also not meant to be a fully thorough upgrade to the documentation, but does fix some of the more egregious precedents that had been set (and, unfortunately, were being subsequently followed). I also certainly do not expect a line-by-line review here , but if you see anything particularly offensive, please let me know!
            Hide
            tjenness Tim Jenness added a comment -

            Lauren MacArthur that looks fine. I've gone through the GitHub project so it now requires that travis check to pass before merging (of course old pull requests will not be able to be merged from now until they are rebased on this ticket).

            Show
            tjenness Tim Jenness added a comment - Lauren MacArthur that looks fine. I've gone through the GitHub project so it now requires that travis check to pass before merging (of course old pull requests will not be able to be merged from now until they are rebased on this ticket).
            Hide
            lauren Lauren MacArthur added a comment -

            Awesome...thanks, Tim!

            Show
            lauren Lauren MacArthur added a comment - Awesome...thanks, Tim!
            Hide
            mschmitz Morgan Schmitz added a comment -

            Looks (very) good to me!

             

            I made a bunch of comments on the PR, but they are all extremely minor - a lot, if not most, related to punctuation in comments. Feel free to ignore these, and in any case okay to merge!

             

            Thank you on behalf of all of us who will be going through the pipe_analysis code in the future!

            Show
            mschmitz Morgan Schmitz added a comment - Looks (very) good to me!   I made a bunch of comments on the PR, but they are all extremely minor - a lot, if not most, related to punctuation in comments. Feel free to ignore these, and in any case okay to merge!   Thank you on behalf of all of us who will be going through the pipe_analysis code in the future!
            Hide
            lauren Lauren MacArthur added a comment -

            Thank you for the expedient yet thorough review!  I addressed all you comments on the PR, including adding the copyright preamble where it was missing (thanks for pointing that out!)  As you saw on Slack, I did my best to poke the bear on the default specification issue as stated in our Dev Guide (but it's not a battle I really want to take on given the push-back...there are clearly cases that I'm not familiar with where this may be bad advice).  Merged and done!

            Show
            lauren Lauren MacArthur added a comment - Thank you for the expedient yet thorough review!  I addressed all you comments on the PR, including adding the copyright preamble where it was missing (thanks for pointing that out!)  As you saw on Slack, I did my best to poke the bear on the default specification issue as stated in our Dev Guide (but it's not a battle I really want to take on given the push-back...there are clearly cases that I'm not familiar with where this may be bad advice).  Merged and done!

              People

              Assignee:
              lauren Lauren MacArthur
              Reporter:
              lauren Lauren MacArthur
              Reviewers:
              Morgan Schmitz
              Watchers:
              Lauren MacArthur, Morgan Schmitz, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.