Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend DataTableLookUp with optional argument: default row #624

Open
wants to merge 6 commits into
base: maintenance/mps20213
Choose a base branch
from

Conversation

AlexeiQ
Copy link
Member

@AlexeiQ AlexeiQ commented Dec 2, 2022

What changed

  • DataTableLookUp is extended with an optional argument defaultRow. This argument allows to specify a default row for the lookup, so in case the looked for value was not found in the searched column, the default row will be returned. Unless the column contains an empty value, then the row of the empty is returned. (If the look up doesn't specify a column for the search, the tables default column is used)
  • Tests for DataTableLookUp and DataTable are cleaned up and improved

Limitations

  • Resolved with 45b6c56:
    When the DataTableLookUp doesn't specify a default column, but uses the tables default column and is about to add the defaultRow argument, the code completion menu suggests the , two times. image Currently it's too hard to resolve this:
    • Either remove the menu item by removing the IDotTarget_TransformationMenu. This has an impact on every IDotTarget
    • Add a transformation menu to the DataTableLookUp, but then other invalid entries are proposed
    • This can even be influenced by MPS changes, when calculations are changed on how the entries for the menu are accumulated

Todos

  • Clarify whether the changed behavior regarding the default is okay (@wsafonov, @mgronover, @AlexeiQ )
  • Clarify the #alias# versus constant-cell question
  • Add release comment
  • Remove Draft, add Reviewers and Prio-label

@AlexeiQ AlexeiQ requested a review from lhartl as a code owner December 2, 2022 21:37
@AlexeiQ AlexeiQ self-assigned this Dec 2, 2022
@AlexeiQ AlexeiQ requested a review from wsafonov as a code owner December 2, 2022 21:37
@AlexeiQ AlexeiQ force-pushed the feature/data-table-look-up-with-default-value-squashed branch from 3af4b42 to 39b5bc5 Compare December 2, 2022 21:41
@alexanderpann alexanderpann marked this pull request as draft December 5, 2022 13:11
@AlexeiQ
Copy link
Member Author

AlexeiQ commented Dec 5, 2022

Extended DataTableLookUp with an optional argument to specify a default row, so if no result is found and the column doesn't contain an empty value, the default row is returned.

@AlexeiQ
Copy link
Member Author

AlexeiQ commented Jan 13, 2023

The lastest changes look good and are working (45b6c56 + 02f1a08).
This also resolves the initial limitation. Great Job!

@AlexeiQ
Copy link
Member Author

AlexeiQ commented Jan 13, 2023

Question: Do we have an convention when to use #alias#-cell and when to use a simple constant-cell?
In this case the lookUpBy and default cells are #alias#, which means, they are editable, but don't delete the node. Regular constant-cells are not editable and can delete the complete node, when the curser is on that cell. lookUpBy is an iDotTarget like oneOf or toList and those use the constant-cell instead of an alias. Thus we have different behavior and I'm wondering if we have some convention when to use what to have a consistent behavior.

@AlexeiQ
Copy link
Member Author

AlexeiQ commented Jan 20, 2023

The default-part of the DataTableLookUp cannot be deleted, when the cursor is placed on the default keyword. Deleting from the = and from eg. first position of the value works fine. As Discussed, this should be fixed as well.

@mgronover
Copy link
Collaborator

mgronover commented Jan 23, 2023

The default-part of the DataTableLookUp cannot be deleted, when the cursor is placed on the default keyword. Deleting from the = and from eg. first position of the value works fine. As Discussed, this should be fixed as well.

This is fixed now. But when the caret is placed right behind val1 (s. below), pressing DEL has no effect.
image
The user has to move the caret to the right side of the , to have the expected delete operation (both DEL and Backspace are working here).

According to (#624 (comment)) the #alias# editor field was exchanged by a constant to make the editor action workable (#alias# with read-only style does not call the required actions) .

@mgronover mgronover changed the title DRAFT: Extend DataTableLookUp with optional argument: default row Extend DataTableLookUp with optional argument: default row Jan 24, 2023
@AlexeiQ AlexeiQ changed the title Extend DataTableLookUp with optional argument: default row Draft: Extend DataTableLookUp with optional argument: default row Jan 24, 2023
@mgronover mgronover marked this pull request as ready for review January 24, 2023 11:02
@mgronover mgronover changed the title Draft: Extend DataTableLookUp with optional argument: default row Extend DataTableLookUp with optional argument: default row Jan 24, 2023
@mgronover mgronover added the PRIO Use for issues/PRs with customer project background label Jan 24, 2023
@arimer
Copy link
Member

arimer commented May 14, 2024

@AlexeiQ, @mgronover in case we need this please change the target branch to at least maintenance/mps20213

@mgronover mgronover changed the base branch from maintenance/mps20211 to maintenance/mps20213 May 14, 2024 14:41
@mgronover mgronover requested a review from arimer as a code owner May 14, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PRIO Use for issues/PRs with customer project background
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants