-
Notifications
You must be signed in to change notification settings - Fork 51
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
JEP 12: Raw string literals #11
Conversation
Thanks for the JEP. In general I think this is a good idea, but there are some logistical issues that will need to be worked out. As you mentioned, "It is reasonable to assume that the most common use case for a JSON literal in a JMESPath expression is to provide a string value to ....(expressions)". If we immediately start warning about this deprecation, this will affect a large number of existing JMESPath users. I'd really prefer to see a period where existing implementations keep their literal implementations as well as implement this raw string JEP. All the docs, examples, tests are updated accordingly but existing users don't have to deal with any warning errors. After this period, warnings (depending on language/library support) are emitted, and then long term eventually removed. What do you think? |
(which can lead to LTS). If the data being escaped was meant to be used | ||
along with another language that uses ``\`` as an escape character, then the | ||
number of backslash characters doubles. | ||
3. Introduces an ambiguous rule to the JMESPath grammar that requires a prose |
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 notion of ambiguity is mentioned several times throughout the JEP. To me, I think this is the strongest motivation for the JEP.
I think it would really help to give at least one specific and concrete example of an expression that is ambiguous. Something like "Given an expression X, we can see from these grammar rules it is ambiguous because it can be parsed as either Y or Z."
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.
Good idea.
Oh and the other logistical thing is versioning. With this being a breaking change in the grammar, it would be helpful to version the specification so an implementation can state which version it implements. The only caveat being I'd like to hold off on a 1.0 version until we get JEP-9 and a version of #2 merged in. Otherwise looks good. |
Do you mean something beyond the JEP number? What specifically do you want me to add to the JEP? |
Keeping the existing implementations compatible sounds good. Once implemented and the docs updated, why would we not start warning right away to give users more time to update their expressions? The expression would still work, but users would know to update their expressions before a 1.0 release. |
Typos are fixed and I added an explanation of the ambiguity. |
My main reason for suggesting it was because many times the end user is not the one that can change the code. For example, a user could be running a bash script that internally uses the AWS CLI and uses the CLI waiter functionality which in its internals uses botocore which uses a JMESPath filter expression with a deprecated literal expression. It's frustrating for the end user to see the deprecation warning because we're the one that needs to make the change and update our waiter config, not them. This is especially frustrating if the change isn't going to be made for a while, as they'll have to continually see a warning they can do nothing about (except ping us). However, I'd be completely fine saying that the each library should figure out its own deprecation/warning strategy. Realistically they're going to be different anyways. The only thing I'd really like to see is just keeping support (warning or not) for some amount of time to allow people to migrate. |
Proposed in jmespath/jmespath.site#11
Thanks for updating. I'd like to move forward this and mark this as accepted. I'll be updating the jmespath.test suite shortly. |
See the attached proposal for the description.
@jamesls, @trevorrowe