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.

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

allow for passing directly the python dict string as response body #59

Merged
merged 18 commits into from
Jan 16, 2025

Conversation

ghyatzo
Copy link
Contributor

@ghyatzo ghyatzo commented Jan 15, 2025

Second PR of the series:
This one is a bit on the nose, but maybe it can be helpful.

Given that the request bodies provided from the download page are valid JSON, instead of using py2ju we can simply use
JSON.parse with this change the usability would be almost exactly like the python API

dataset = "reanalysis-era5-single-levels"
request = """{
    "product_type": ["reanalysis"],
    "data_format": "grib",
    "download_format": "unarchived"
}""" # notice the """, this is a valid JSON String.

CDSAPI.retrieve(dataset, request, target)

So far I still haven't encountered a request in a format that wasn't already valid JSON.

In any case, it is still backwards compatible with using py2ju.

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Could you please comment on how this plays with the current CDSAPI.py2ju helper? Should the helper be deprecated?

src/CDSAPI.jl Outdated Show resolved Hide resolved
src/CDSAPI.jl Outdated Show resolved Hide resolved
@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 15, 2025

The current usage of the py2ju helper is untouched. Since the call would be
retrieve(dataset, CDSAPI.py2ju(request), target) instead of the proposed retrieve(dataset, JSON.parse(request), target).

Both calls will dispatch to retrieve(name, params::AbstractDict, filename) (and the resulting Dict should be the same)

Effectively yes, py2ju does the same job as JSON.parse.
I would maybe still keep it around to be backward compatible with scripts that use it. maybe with a deprecation warning?

@juliohm
Copy link
Member

juliohm commented Jan 15, 2025

A deprecation warning is a good idea. We should update the README with the new recommended method, which is a simple copy/paste of the Python string. The README could also teach users that JSON.parse(pythonstring) produces a Julia dictionary for programmatic manipulation of the request body.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
will do the rest.
Python dictionaries can be valid JSON strings, and the CDS requests builder is kind enough to make it so, (make sure the request string does not contain single quotes, but only double quotes)

Therefore simply calling `JSON.parse(request)` will return a valid Julia dictionary. This is done for you automatically as the call above will be translated into `CDSAPI.retrieve(dataset, JSON.parse(request), "download.nc")`.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we should always recommend the method with string for end-users, and leave this technical detail about dicts for developers as comments in the source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agree, I'll change accordingly.

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

I've updated the retrieves docstring. Let's try to finish this PR first before #58. We can release a patch for this given that it is non breaking, and then consider a minor release for the scoped values feature.

README.md Outdated Show resolved Hide resolved
@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

I've refactored the tests to simplify maintenance, and removed the testset related to py2ju, which now redirects to a new CDS.parse helper.

The tests are failing due to an undefined endpoint variable.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

That's weird, The endpoint line got clobbered somewhere. Maybe while moving from one pr to the other...

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

The tests are now failing due to changes related to py2ju. Appreciate if you could adjust the request strings in the test sets to the new format, assuming it never contains single quotes.

@ghyatzo
Copy link
Contributor Author

ghyatzo commented Jan 16, 2025

test pass locally with that last commit.

@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

Thank you! 🙂

@juliohm juliohm merged commit 6b5be92 into JuliaClimate:master Jan 16, 2025
0 of 6 checks passed
@juliohm
Copy link
Member

juliohm commented Jan 16, 2025

Released another patch JuliaRegistries/General#123128

@ghyatzo ghyatzo deleted the direct-json branch January 16, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants