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.
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
chore: Remap v1 RFC #3134
chore: Remap v1 RFC #3134
Changes from 7 commits
fbef810
07804ac
c29ac05
946fbeb
86cca1e
e117e40
b9941ab
2213383
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Everything else here is great, but this still feels funky to me. I understand why it's this way, the left-hand side is the "target" and the right-hand side is the "action", but the traditional function syntax seems more intuitive. Going with our
jq
theme, this would just bedel(.foo)
:Does this introduce significant complexity? I'm also curious to get @lukesteensen's opinion.
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.
Yeah, this is another case where I think we can try to avoid keywords and use something less ambiguous like a builtin function.
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.
Got options here, bloblang and IDML use
foo = deleted()
, so it's a function rather than a keyword which we could copy. Implementing deletions as a right-hand query allows you to use it in all the same ways as regular mappings, which means they can be conditional:And also mapped in an iterator in order to remove elements from arrays, etc:
However, iterators aren't part of the RFC scope and if we're keeping the spec minimal then I think it's reasonable not to support those cases. It's easy to add a
del(.foo)
left-hand function, but if we want to enable conditional deletions the same as conditional mappings then we'd need to implementif
as a statement you can put on the left-hand side:At which point we need to decide whether we bother implementing it as a right-hand expression as they'd be parsed and implemented separately. Doing so is useful for expanding the language later, but not so much if we're keeping the language minimal.
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 see what you're saying, and it makes sense, but from a UX standpoint I find the following much more intuitive:
I feel like I might be suggesting something that is fundamentally at odds with the purpose of this language (performance). If that is the case, then I'd prefer to go with your proposal, otherwise, I find the above clearer. Performance is a key requirement of this language and I don't want to sacrifice that.
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.
Ok, apologies for the back and forth. I've been trying to build consensus off line on the syntax proposed here. Just confirming was we settled on for this RFC:
This would:
foo
key with the value"bar"
.baz
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.
There are two important factors here, readability and scalability.
Readability is a tricky one. I definitely agree that jq can be hard to understand, but that's largely a non-issue if the user is already familiar with jq from somewhere else. Given that it's pretty popular, it's hard to say how those balance out.
Scalability is one where we need to think about the target use cases. If we're expecting for this transform to handle mostly light reshaping, then scalability is less of an issue. Users will likely have the opportunity to master the basics before diving into something more complicated and may not mind spending a bit more time to figure out something they recognize is pretty complicated. On the other hand, if we expect a meaningful number of users to jump straight into larger mapping declarations, it's more important that we have something that scales smoothly to that size and level of complexity.
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.
Agree with all of the above regarding Tremor script. I think it's an impressive feature of Tremor, but would like to avoid it for the reasons you listed.
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.
Agree, I think this will aid us in support.
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.
The value exceeds the effort in my opinion.
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.
Agree with everything you've laid out as a rationale. I am big fan of starting with a small experiment.