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
Fixed inconsistencies between
listen
andemit
#943Fixed inconsistencies between
listen
andemit
#943Changes from 14 commits
e1d565a
ec69fd1
cd7a0b9
93dcd34
21eac79
5bf4ad7
e605f67
0579a94
302fe64
333c83c
2ac77b4
555a0f7
fcd10fd
04cd3be
ebd2e10
821b56f
233accb
876bf2c
839492e
92562e0
caec8ab
97d59e7
7c749d3
fceb957
34c678b
1ecec3f
0aaead4
98d73ee
f1c32af
43c3d15
c49c263
5961814
52ad683
ca94f62
624d402
7fcc3ca
808d72b
434e0eb
85c43c8
538a0a6
0ea6dcb
151392d
b7236c8
87758fc
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.
I thought I was getting crazy since I couldn't see my previous comments from the other PR. Now I see you reopened it.
Regarding this property, since we are now defining that it can be regexp, runtime expressions, or
eventProperties
, this should reflect in the schema.So this
with
should be anOneOf
there amongst these types.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.
Sorry for the mix-up. When I rebased the branch, GitHub closed my previous PR. I opened a new one and realized afterward that I could have simply reopened the previous one after I readded my commits.
I might be mistaken, but I was under the impression that the statement "Supports both regular expressions and runtime expressions" applies to the properties of
with
, not the object itself. That said, there might be room for discussion about how and where we can use a runtime expression to construct an entire object, but that might be a broader topic than what is addressed in this PR.@cdavernas @fjtirado @matthias-pichler-warrify, what are your thoughts on this?
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 would understand this to mean that the individual properties under
with
(such asid
,source
, ...) should each support literals, regex and runtime expressions. These are all strings so we would just produce a union of typestring | string | string
which doesn't seem necessary.I would keep the
with
object an object.regarding regular expressions: How would an implementor determine if a given string is a literal or a regular expression though?
For example if the filter was specified as
if the string was interpreted as a literal only
com.example
would match. As a regexcomaexample
andcom.example.foo
would also match! Are we planning on adding a regex syntax (JSON schema support the ECMA 262 syntax withformat: regex
) or object? LikeMaybe we should just stick to runtime expressions and use the
test
function: https://jqlang.github.io/jq/manual/#regular-expressions ?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.
That's a good point. It's a very specific pain point considering regular expressions are only supported in
listen
tasksevent filters
.My understanding was that the value was either a string or a runtime expression. If it's a string, then it's regarded as a regular expression (regexp). But it's not very clear, can introduce undesired matches when not properly escaped, and probably have an bad impact on performance (I would assume string equality to be more performant than regexp, in turn more performant than jq/js expressions.)
For the performance reason I just mentioned (without any proof though, it's only a feeling ^^'), I would rule out relying only on jq/js for processing regexp. Therefore, we need to either consider any string as a regexp or find a way to discriminate the regexp. I must admit I'm not convinced by neither adding a property nor using a specific format but I don't have a better option to suggest. Therefore, I would rather go for the specific format rather than the extra property.
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.
My intuition is that the performance difference between string equality and regex matching is negligible compared to
jq
expressions which are much slower. I however would not have a problem with taking the performance hit and only using runtime expressionsIf we support regex I would also say that we should use a format such as
/<regex>/
. We would however not be able to rely on theformat: regex
from json schema since this does not have any delimiters