cleaner selection get methods

[bmschmidt]

Split the new qid-getting method back off

---

Important

Refactor get() method in DataSelection and SortedDataSelection by splitting logic into get(), getQid(), and getBase() for improved clarity.

• Refactoring:
  • Split get() method in DataSelection and SortedDataSelection into get(), getQid(), and getBase() for clarity.
  • get() now calls getBase() with returnQid set to false.
  • getQid() added to return a tuple of [tile index, row index] by calling getBase() with returnQid set to true.
  • getBase() contains the core logic for retrieving elements based on index.

^{This description was created by for 8489ba4. You can customize this summary. It will automatically update as commits are pushed.}

[bmschmidt]

• qid refactors #198
• cleaner selection get methods #199 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

The JSDoc for the get method still references the returnQid parameter which has been removed from this method. Since this functionality has been split into the separate getQid method, this parameter description should be removed from the get method's documentation to accurately reflect the current implementation.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 5b0f5a9 in 1 minute and 42 seconds. Click for details.

• Reviewed 69 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/selection.ts:820

• Draft comment:
  Nice separation of the API by splitting get() and getQid() into two distinct methods. This improves readability and adheres to DRY principles.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 70% None

2. src/selection.ts:828

• Draft comment:
  Refactoring the shared logic into the private getBase() method is a good move. Note the improved variable naming (using 'rix' for the row index) for clarity. Consider whether the linear scan through tiles might become a performance bottleneck for many tiles and if a cached cumulative count or binary search could help.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 70% None

3. src/selection.ts:847

• Draft comment:
  The caching pattern using 'cachedWhiches.get([relevantTile])' relies on wrapping the tile in an array. Ensure that FifoTupleMap is intended to use arrays as keys; if not, consider using the selection tile object directly to avoid potential key-matching issues.
• Reason this comment was not posted:
  Confidence changes required: 66% <= threshold 70% None

Workflow ID: wflow_kvCPXJIoXWtbU6aR

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 8489ba4 in 1 minute and 6 seconds. Click for details.

• Reviewed 101 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/selection.ts:792

• Draft comment:
  The JSDoc for the new get method still mentions a returnQid parameter, even though the method no longer accepts it. Please update the comment so it only documents that get returns a row, and that getQid returns the [tile index, row index] tuple.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. src/selection.ts:1110

• Draft comment:
  The get and getQid methods in SortedDataSelection duplicate the same error‐checking pattern as in DataSelection. If their behavior is identical (aside from the different getBase implementations), consider reusing the inherited versions to reduce duplication or add a brief comment explaining why they must be redefined.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_aJG4rWxUuSZO4Obk

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[bmschmidt]

Suggested change

* @param returnQid if true, returns a tuple of [tile index, row index] instead of the row data

[RLesser]

LGTM

[bmschmidt]

Merge activity

• May 29, 5:46 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 29, 5:46 PM UTC: @bmschmidt merged this pull request with Graphite.