-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add --json-file/-j option to read from file w/ json contents #311
base: master
Are you sure you want to change the base?
Conversation
brew install...
not leaking
cleanup action cleanup trurl.1 cleanup trurl.1
okay I need to figure out whats going on with the windows builds, the cygwin was was working on my local branch until very recently, might be something i changed on the way we are doing the file i/o. I will take a closer look this evening. Edit: Okay, i got the cygwin build working. I just need to add libjson-c to the curl-for-win build. @vszakats would this be done by manually compiling and linking it in here: https://github.com/curl/curl-for-win/blob/main/trurl.sh ? |
It's a new dependency, kind of a big deal to add support for one in general. One approach is to make it a build option, disabled by default, so existing trurl builds (including curl-for-win) continue to work without modification. Another option is to delete/disable the curl-for-win CI job till I (or someone else) get around adding support for it. A 3rd one is to add an option to disable this feature, and I update curl-for-win to set it. |
...that said, have you thought of using a JSON parser that's a self-contained C89 source (with a fitting license)? It's still a dependency, but perhaps easier to integrate and carry. |
I was looking a lot at https://github.com/DaveGamble/cJSON, but I didn't like how it looks and json-c has some nice features. Both this and json-c are in vcpkg, freebsd repos, and most linux distro repositories so i was hoping it wouldn't be too much work either way for maintainers. If folks want a smaller ANSI c json library instead of the one I chose, I think that's a fair request and I can start working this again. for now, I will put it behind a compile time flag as you suggested Edit: |
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
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.
We made the -f
function possible to work as a filter, as in it reads URLs and outputs the results in the mean time. This JSON reader reads the entire file before it acts on it, which makes it not work as a filter and if you want to work an a huge number of JSON URLs this will allocate a lot of memory...
@bagder, i've made a few big modifications. Mainly I made it so it reads one json object at a time, so it now only allocates as much memory as required to parse the largest single json object, instead of attempting to read until the of the file. I also made it so it reads in a fixed buffer on the stack, and then allocates more data on the heap if need be as you suggested. |
Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>
This adds
--json-file
as an option to read from a file that contains a json representation of the urls. It works very similarly to--url-file
. The--json-file
input structure is designed to match roughly what the output of--json
produces. the simplest json object possible is:You can pass an arbitrary number of these url JSON objects and perform operations on them like you would with any other url in trurl.
Something that I did that may be up for debate is how the query is handled. I chose to make the query only be able to be set by an array of "keys" and "values" (the "params" section of --json") and it will throw a warning if the "query" component of "parts" is set.
A few examples:
Currently the feature is disabled by default, do turn it on you need to add
TRURL_JSON_IN
to your environment to enable it.fixes: #291