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

design Array fields for table version 1

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:

      Description

      While we're trying to eliminate the need for compound fields in afw::table, Arrays present a few problems. We could use FunctorKeys, the way we plan to use other compound fields, but here we need to guarantee that the per-element keys are contiguous, and we also be able to support views. We also need to determine the naming scheme. Finally, we need to make sure these work with aliases and slots.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment -

          Ready for review. Perry, could you take a look?

          Code is on u/jbosch/DM-836 of afw, and there's a GitHub Pull Request page you'll find linked on the right side of the issue page that shows the diff and all the commits involved, and gives the the ability to comment on individual lines in the diff (just click to the left of any line). Please make your comments there, and let me know if you have any questions about how the reviews on GitHub work.

          Show
          jbosch Jim Bosch added a comment - Ready for review. Perry, could you take a look? Code is on u/jbosch/ DM-836 of afw, and there's a GitHub Pull Request page you'll find linked on the right side of the issue page that shows the diff and all the commits involved, and gives the the ability to comment on individual lines in the diff (just click to the left of any line). Please make your comments there, and let me know if you have any questions about how the reviews on GitHub work.
          Hide
          jbosch Jim Bosch added a comment -

          Taking this out of review temporarily; I've noticed a problem that I need to fix first.

          Show
          jbosch Jim Bosch added a comment - Taking this out of review temporarily; I've noticed a problem that I need to fix first.
          Hide
          jbosch Jim Bosch added a comment -

          Fixed the bugs, so I'm putting it back in review. New commits should appear on the GitHub PR mentioned above.

          Show
          jbosch Jim Bosch added a comment - Fixed the bugs, so I'm putting it back in review. New commits should appear on the GitHub PR mentioned above.
          Hide
          pgee Perry Gee added a comment -

          I put my comments on GitHub. I should have listed linenos. and didn't, but let me know if it isn't obvious.

          Show
          pgee Perry Gee added a comment - I put my comments on GitHub. I should have listed linenos. and didn't, but let me know if it isn't obvious.
          Hide
          jbosch Jim Bosch added a comment -

          Summary of the review:

          • Request for code comments in tests (done).
          • Trivial doc fixes (done).
          • Raised concern about behavior of BaseRecord.__getitem__ when passed an invalid `Key`; I looked closer at the code, and found that a lower-level call already included a check, so just letting that propagate up will do what we want.
          • A question about the usefulness of the constructor that takes an ArrayKey; my reply is that it's useful in order to give the user more control over documentation and units for individual Keys in the array, and to provide a more efficient way to reconstruct an ArrayKey when we already have the Keys in hand.
          Show
          jbosch Jim Bosch added a comment - Summary of the review: Request for code comments in tests (done). Trivial doc fixes (done). Raised concern about behavior of BaseRecord.__getitem__ when passed an invalid `Key`; I looked closer at the code, and found that a lower-level call already included a check, so just letting that propagate up will do what we want. A question about the usefulness of the constructor that takes an ArrayKey ; my reply is that it's useful in order to give the user more control over documentation and units for individual Keys in the array, and to provide a more efficient way to reconstruct an ArrayKey when we already have the Keys in hand.
          Hide
          robyn Robyn Allsman [X] (Inactive) added a comment -

          I was just in the middle of reviewing it. I was still trying to get
          through the code though, and hadn't yet noticed any problems.

          Show
          robyn Robyn Allsman [X] (Inactive) added a comment - I was just in the middle of reviewing it. I was still trying to get through the code though, and hadn't yet noticed any problems.

            People

            Assignee:
            jbosch Jim Bosch
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Perry Gee
            Watchers:
            Jim Bosch, Perry Gee, Robyn Allsman [X] (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.