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

Change fake source insertion tasks to use generators rather than appending to lists.

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
    • Story Points:
      4
    • Epic Link:
    • Sprint:
      DRP F19-5, DRP F19-6 (Nov)
    • Team:
      Data Release Production

      Description

      Generators will probably use less memory than adding images to a list. Update the fake source insertion code to use these instead.

        Attachments

          Activity

          Hide
          sophiereed Sophie Reed added a comment -

          This was done as a pair coding exercise with Arun Kannawadi.

          While doing it we noticed that adding the fake tasks to bin.src had not been committed and that some updates were needed to bring the code into line with the new gen 3 interface. As these were both small changes they were addressed on this ticket.

          Show
          sophiereed Sophie Reed added a comment - This was done as a pair coding exercise with Arun Kannawadi . While doing it we noticed that adding the fake tasks to bin.src had not been committed and that some updates were needed to bring the code into line with the new gen 3 interface. As these were both small changes they were addressed on this ticket.
          Hide
          sophiereed Sophie Reed added a comment -

          If you have a second please could you look at this?

          Show
          sophiereed Sophie Reed added a comment - If you have a second please could you look at this?
          Hide
          swinbank John Swinbank added a comment -

          I hate drive-by reviews, but since I happened to notice this in passing...

          • Please split this into multiple commits. Adding the scripts into bin.src shouldn't be on the same commit as replacing lists with generators.
          • Arun Kannawadi, please register the e-mail address you're using to commit with GitHub (so that it automatically links your commits to your profile).
          • This change seems worrying — not because it's wrong, but because it seems to imply that none of our test cases are even importing this code. Is that right? If so, you definitely don't need to fix that on this ticket, but please file another ticket to do so in future.

          Sorry again for interfering!!

          Show
          swinbank John Swinbank added a comment - I hate drive-by reviews, but since I happened to notice this in passing... Please split this into multiple commits. Adding the scripts into bin.src shouldn't be on the same commit as replacing lists with generators. Arun Kannawadi , please register the e-mail address you're using to commit with GitHub (so that it automatically links your commits to your profile). This change seems worrying — not because it's wrong, but because it seems to imply that none of our test cases are even importing this code. Is that right? If so, you definitely don't need to fix that on this ticket, but please file another ticket to do so in future. Sorry again for interfering!!
          Hide
          nlust Nate Lust added a comment -

          Don't worry John Swinbank, I was just looking at this, and was bringing up the same things.

          Show
          nlust Nate Lust added a comment - Don't worry John Swinbank , I was just looking at this, and was bringing up the same things.
          Hide
          swinbank John Swinbank added a comment -

          You're my hero, Nate!

          Show
          swinbank John Swinbank added a comment - You're my hero, Nate!
          Hide
          nlust Nate Lust added a comment -

          You did save me a lot of typing things out, so thanks for that

          Show
          nlust Nate Lust added a comment - You did save me a lot of typing things out, so thanks for that
          Hide
          kannawad Arun Kannawadi added a comment -

          Thanks a lot for all the comments Nate Lust and John Swinbank. Very much appreciated. They must all be addressed now.

          Sophie Reed had mentioned someone else ran the fake source ingestion, but I am not sure how it could have worked. Definitely worth strengthening the unit tests to improve the coverage there.

          Show
          kannawad Arun Kannawadi added a comment - Thanks a lot for all the comments Nate Lust and John Swinbank . Very much appreciated. They must all be addressed now. Sophie Reed had mentioned someone else ran the fake source ingestion, but I am not sure how it could have worked. Definitely worth strengthening the unit tests to improve the coverage there.
          Hide
          kannawad Arun Kannawadi added a comment -

          I believe it's ready to be merged in the `master` branch now. I could perhaps wait for Sophie to come back from her vacation.

          Show
          kannawad Arun Kannawadi added a comment - I believe it's ready to be merged in the `master` branch now. I could perhaps wait for Sophie to come back from her vacation.

            People

            Assignee:
            sophiereed Sophie Reed
            Reporter:
            sophiereed Sophie Reed
            Reviewers:
            Nate Lust
            Watchers:
            Arun Kannawadi, John Swinbank, Nate Lust, Sophie Reed, Yusra AlSayyad
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.