refactor(infer): consolidate two main functions #617
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
patricebender
requested review from
danjoa,
johannes-vogel,
stewsk and
BobdenOs
as code owners
|
The stakeholder tests ( |
stewsk
approved these changes
May 10, 2024
|
@patricebender @stewsk this pr breaks cds ci, i'll revert for now |
sjvans
added a commit
that referenced
this pull request
May 12, 2024
Reverts #617 @patricebender @stewsk #617 breaks cds ci
patricebender
added a commit
that referenced
this pull request
Nov 15, 2024
pseudo paths were leading to a dump if they were used in an expand on an association which follows them in its filter expression. We need to revisit #617, as there is a lot of duplicate logic in two main functions of `infer` --> get rid of `attachRefLinksToArg`!
patricebender
added a commit
that referenced
this pull request
Nov 18, 2024
pseudo paths were leading to a dump if they were used in an expand on an association which follows them in its filter expression. We need to revisit #617, as there is a lot of duplicate logic in two main functions of `infer` --> get rid of `attachRefLinksToArg`! --------- Co-authored-by: Johannes Vogel <[email protected]>
2 tasks
patricebender
added a commit
that referenced
this pull request
Nov 28, 2024
Rework #617 when the `infer` function was developed, it grew quite fast and some functions were tailor made for very specific use cases. While separation of concerns is a good practice w.r.t. maintainability, the wheel must not be re-invented for each minor deviation in behavior. That being said, the two function `inferQueryElement` and `attachRefLinksToArg` are doing essentially the same. They both walk an `arg` which is - generally spoken - an expression. That might be a simple `ref`, `func`, `val` or a (nested) `xpr`. - `inferQueryElement` was used for the `columns` of a query. Each column was resolved to a query element. All elements together formed the set of query elements. The other main part of the function is the attachment of the infamous `$refLinks` next to each `ref` array. Moreover, there was a flag `insertIntoQueryElements` which could be set to `false`, if the `arg` must _not_ be inserted into the queries elements. This is the case for nested `xpr`, the `where`, `having`, `with`, `groupBy` and `orderBy` clause of the query. - `attachRefLinksToArg` does exactly what the name implies, it attaches `$refLinks` to an `arg` but does not resolve the `arg` to a query element. This function was used for the `from` clause of the query. It is obvious that both functions have significant shared logic. This change removes the usage of the `attachRefLinksToArg` function. - [x] stakeholder tests are all green with this branch → cap/stakeholder-tests/actions/runs/9482613 - [x] cds tests are all green with this branch → /cap/cds/actions/runs/9483245
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when the
inferfunction was developed, it grew quite fast and some functions were tailor made for very specific use cases. While separation of concerns is a good practice w.r.t. maintainability, the wheel must not be re-invented for each minor deviation in behavior.That being said, the two function
inferQueryElementandattachRefLinksToArgare doing essentially the same. They both walk anargwhich is - generally spoken - an expression. That might be a simpleref,func,valor a (nested)xpr.inferQueryElementwas used for thecolumnsof a query. Each column was resolved to a query element. All elements together formed the set of query elements. The other main part of the function is the attachment of the infamous$refLinksnext to eachrefarray. Moreover, there was a flaginsertIntoQueryElementswhich could be set tofalse, if theargmust not be inserted into the queries elements. This is the case for nestedxpr, thewhere,having,with,groupByandorderByclause of the query.attachRefLinksToArgdoes exactly what the name implies, it attaches$refLinksto anargbut does not resolve theargto a query element. This function was used for thefromclause of the query.It is obvious that both functions have significant shared logic. This change removes the usage of the
attachRefLinksToArgfunction.