-
Notifications
You must be signed in to change notification settings - Fork 838
Proposal of new declarative match step #3232
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
base: 3.8-dev
Are you sure you want to change the base?
Proposal of new declarative match step #3232
Conversation
Thanks for putting together the proposal @andrii0lomakin. As discussed on the devlist, I am fully in favour of the proposed changes. VOTE +1 Note: The build is failing due to a missing license header. Pasting the standard license header to the top of the new file should resolve this:
|
Update specification for the declarative `match()` step in Gremlin to include licensing information and converting to asciidoc
Well I am a bit puzzled because now it contains license. |
To prevent query injection and improve performance by enabling query | ||
plan caching, parameterized queries are supported. Parameters are | ||
supplied using the existing `with()` modulator with a special key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency with a steps like math
i think we'd prefer by
rather than with
. That also removes the conflict with configurations for match
like the aforementioned: `with("queryLanguage", "GSQL")``
hmm, maybe i should say that a different way. i suppose i'd like to see with
remain a modulator for step configuration and less for passing arguments. by
could work in some ways (not quite like math
). maybe we could do match(String, Map)
so that the parameters are just a map of key/value pairs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the last one.
@Cole-Greer, @lpld WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match(String, Map)
sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the variant with Map argument
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.8-dev #3232 +/- ##
==========================================
Coverage ? 76.79%
Complexity ? 14946
==========================================
Files ? 1159
Lines ? 72489
Branches ? 8035
==========================================
Hits ? 55671
Misses ? 13811
Partials ? 3007 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposal a new declarative match step that should replace the current procedural version.