-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Replace JSONPath with jq
in jsondocck
#143089
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: master
Are you sure you want to change the base?
Replace JSONPath with jq
in jsondocck
#143089
Conversation
it's unnecessary for private crates
the deleted lines are duplicates of the lines 36 & 37
there are no insignificant whitespaces or comments, it's a single line
- directives were collected before processing, but there seems to be nothing preventing them from checking right away - the unified error message style follows the style of short diagnostics from the compiler (almost the same was used on the deleted line 39)
i tried to keep the original code structure, but it's an almost entire rewrite. thankfully, there are more deletions than insertions, i expect jq to be incredibly powerful to supersede previous set of directives. no tests yet, but it seems to be working. i also simplified the `LINE_PATTERN` regex and added a comment for each section. jaq's guts are a bit arcane to me, but it seems to be only related to its setup, a resulted API turned out to be pretty straightforward
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #143337) made this pull request unmergeable. Please resolve the merge conflicts. |
(cc @aDotInTheVoid) |
Sorry for taking so long to review this. Thanks for working on it, especially the repetitive test migration.
What do you mean by this? The idea was to use the transpiler to migrate all the assertions in the tests, leaving them in jq. New contributors should never need to know it existed.
I think this is a quite bad idea. It means we loose any sort of good error reporting. With this PR at the moment, we can only say that a check failed, but not why (in the case that the query matched the wrong value). Having this is really important I think. |
This is an ongoing effort to close #142479. I've decided to take a hard path and rewrite
rustdoc-types
's entire testsuite. I don't think it'll be a good idea to construct a JSONPath-jq
transpiler because it'll look really awkward for new contributors, JSONPath's limitations +jaq
's own quirks may create a lot of questions that even current maintainers would've no answers for without digging intojsondocck
's internals.I expect
jsondocck
to have just 2 directives:jq
andarg
. Thanks tojq
's flexibility, these 2 can do everything that the current directive set can and much more (actually, too much, I left some comments to opt out some dependencies when it'll be possible onjaq
's side). I'm trying to actively utilizejq
's features instead of just blindly rewriting existing assertions, so there should be more deletions than additions.When I'll be done, I'll try to rebase and split my work to make it more easier to review. So far
jsondocck
is already functional, so you can check out how it may look like and give some feedback if you want. I'll commit updated tests folder by folder.