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

Refactor to allow eager loading, or no proxies at all (1.0.0 beta) #43

Merged
merged 22 commits into from
Oct 28, 2022

Conversation

gazpachoking
Copy link
Owner

@gazpachoking gazpachoking commented Oct 10, 2022

A couple things going on in this PR.

The main interface has been changed from JsonRef.replace_refs to a top level function replace_refs. Just feels like it makes more sense, and I can't remember why it was a class method to start with. Also fixes (#22). JsonRef.replace_refs is deprecated, but still works for now.

jsonloader has had the caching removed, and turned into a plain function rather than an instance of a special class. Caching of ref results is now handled directly by replace_refs. The previous caching behavior was a bit muddled, and this makes it easier for custom loaders to be implemented without worrying about caching. There is now no way to turn off caching (which was mostly the case previously as well, some of the caching was required for basic functionality.) If you want to pre-cache some URIs, it can be done by creating a custom loader function that loads from said cache.

Due to multiple issues filed, three new options were added to replace_refs:

  • The lazy_load option. When set to False, all references will be eagerly loaded. This can be useful because any errors will happen right on initial load, rather than when the reference is eventually accessed.
  • The proxies option. When set to False, the resulting document won't have JsonRef objects in it at all, they will be replaced by the actual referred to data (Support $ref replacement without the proxies #9, Feature Request: when serializing, replace references with referents. #31). This can be nice when using with another library that doesn't work properly with the proxy objects, e.g. using json.dump to output the document with the refs replaced (How to save to regular JSON without ref? #41). If used, this will of course mean you will be unable to output the document again with the refs intact.
  • The merge_props argument. This was based on Preserve extra sibling attributes #35, and allows extra properties of json reference objects to be merged in to the document loaded by the reference. This is not part of the JSON reference spec, so is turned off by default.

I'm hoping for some feedback on this PR, and testing by anyone using the repo. I might make a beta release on pypi for easier testing if needed. Open questions:

  • Is this a good plan?
  • Should the default values for lazy_load or proxies be changed? My inclination would be to make the default still use proxies, but to eagerly load, so that all errors happen immediately.
  • I added a walk_refs helper function, which is used internally to support these two new options this will recursively run a function on any JsonRefs in an object, (and optionally replace them with the result of the function.) Is this useful to make a public helper function, or is it not needed since the new options will use it directly?

@gazpachoking
Copy link
Owner Author

gazpachoking commented Oct 19, 2022

I made a beta release on pypi. Feedback appriciated https://pypi.org/project/jsonref/1.0.0b3/

@gazpachoking
Copy link
Owner Author

gazpachoking commented Oct 20, 2022

@gazpachoking
Copy link
Owner Author

New beta published.

@gazpachoking gazpachoking changed the title Refactor to allow eager loading, or no proxies at all Refactor to allow eager loading, or no proxies at all (1.0.0 beta) Oct 24, 2022
@pkarman
Copy link
Contributor

pkarman commented Oct 24, 2022

I kicked the tires on the new branch. merge_extra_properties does not appear to be passed in kwargs in the .load and .loads convenience wrappers.

Add new replace_refs kwargs to load utility functions
@gazpachoking
Copy link
Owner Author

I kicked the tires on the new branch. merge_extra_properties does not appear to be passed in kwargs in the .load and .loads convenience wrappers.

Thanks! And good catch. Added the new kwargs to those functions and pushed another beta.

@pkarman
Copy link
Contributor

pkarman commented Oct 24, 2022

merge_props works great! thanks for much for accommodating the request.

@gazpachoking
Copy link
Owner Author

I'll give it another day or two for feedback before I cut the actual release.

@gazpachoking gazpachoking merged commit 421f22d into master Oct 28, 2022
@gazpachoking gazpachoking mentioned this pull request Oct 28, 2022
@jpmckinney
Copy link
Contributor

merge_props=True is great! I no longer need my own code to do that 😄 And lazy_load=False saves me from having to call repr(data) when checking that the references resolve.

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.

3 participants