Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow string comparison #49

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Allow string comparison #49

merged 1 commit into from
Feb 14, 2022

Conversation

johnallen3d
Copy link
Contributor

This library currently only supports "comparison" of numberic values. This appears to match the JMESPath spec but is not consistent with other implementations (for example: jmespath.py).

Resolves: #47

@johnallen3d johnallen3d force-pushed the feature/allow-string-comparison branch from 9c0cce6 to 2d4691f Compare January 31, 2022 13:52
@johnallen3d
Copy link
Contributor Author

Hello @alextwoods, I just noticed that you've recently pushed a few updates to this repository. I've rebased this PR with main and pushed. Any chance of getting it merged? FWIW — we've been using a privately published version of the gem with these changes for ~2yrs now without issue.

@alextwoods alextwoods self-assigned this Jan 31, 2022
alextwoods
alextwoods reviewed Jan 31, 2022
View reviewed changes
Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the code itself looks reasonable - my main concern is that the JMESPath spec is explicit that comparisons operations are valid only for numbers and other types must return null.

Do you know if other implementations support string comparison?

lib/jmespath/nodes/comparator.rb Outdated Show resolved Hide resolved
@johnallen3d
Allow string comparison
7f0a880 
This library currently only supports "comparison" of numberic values.
This appears to match the JMESPath spec but is not consistent with other
implementations (for example:
[`jmespath.py`](jmespath/jmespath.py#126)).

Resolves: jmespath#47
@johnallen3d johnallen3d force-pushed the feature/allow-string-comparison branch from 2d4691f to 7f0a880 Compare January 31, 2022 18:12
@johnallen3d
Copy link
Contributor Author

Thanks for the quick response @alextwoods. jmespath.py does currently support this type of comparison (not 100% sure about others). Also, when I was working on this originally, https://jmespath.org/ did also work this way (maybe it's driven by jmespath.py?). Also, I may have misinterpreted the comment here and here but I thought @jamesls was saying he was going to add this type of comparison to the spec.

FWIW - I removed the debug code and amended the commit.

@alextwoods
Copy link
Contributor

Thanks for the patience - I'm working with @jamesls on possible updates on the spec. The issue is in defining the string comparisons in a way that implementations will be consistent (this gets complex, see: https://unicode.org/reports/tr10/). One possibility is to just say its utf8 encoding byte comparison.

@alextwoods
Copy link
Contributor

I'm going to go ahead and merge this and release a new version of jmespath - we'll work on getting the spec updated as well. Thanks for the contribution!

@alextwoods alextwoods merged commit 4b8773a into jmespath:main Feb 14, 2022
@alextwoods
Copy link
Contributor

This has been released as jmespath 1.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alextwoods alextwoods alextwoods left review comments

Assignees

@alextwoods alextwoods

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Broken "Date" Filter Support
2 participants
@johnallen3d @alextwoods