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

util: Support jsonencode/jsondecode as regular functions #24

Merged
merged 1 commit into from
Dec 24, 2019
Merged

util: Support jsonencode/jsondecode as regular functions #24

merged 1 commit into from
Dec 24, 2019

Conversation

apjanke
Copy link
Collaborator

@apjanke apjanke commented Dec 20, 2019

What do you think about supporting jsonencode/jsondecode when implemented as regular functions, in addition to just when they're built-ins? This would allow the jsonencode/jsondecode from jsonstuff (https://github.com/apjanke/octave-jsonstuff) or any other Matlab-compatible implementation to be used, in addition to JSONio. This would be nice for me because I'd rather use jsonstuff than JSONio. And there's a chance that jsonencode/jsondecode will eventually get merged into Octave core from one of these libraries, but might be regular functions instead of builtins there, so this would be forward-looking for future Octave versions. (Also, Matlab could always change the jsonencode/jsondecode implementation to use regular M-file wrapper functions, which would break the current detection.)

@gllmflndn
Copy link
Collaborator

Yes. JSONio is only here temporarily until we can assume Matlab is recent enough to have jsonencode/jsondecode and an agreement is reached in Octave:
https://savannah.gnu.org/bugs/?53100
Shouldn't the test with exist also look for an M-file (ie not only 3)? It would also perhaps be better if the checks were only done once with the outcome stored in a persistent variable (as a function handle).
Having jsonstuff pushed into Octave before the 6.1 release would be great. If there isn't a quick agreement of which C++ library to rely on, jsonencode could still be added as I guess everyone implemented it in pure M-code.

This allows the jsonencode/jsondecode from jsonstuff (https://github.com/apjanke/octave-jsonstuff)
or any other Matlab-compatible implementation to be used, in addition to JSONio.
@apjanke
Copy link
Collaborator Author

apjanke commented Dec 20, 2019

You're right; it should be looking for both 3 and 5. I have amended this PR to do so, and to store the outcome in a persistent variable. (Though maybe you effectively wanted the entire if/elseif/.../end chain logic and all those tests stored in a persistent.)

@robertoostenveld robertoostenveld merged commit caf4d75 into bids-standard:master Dec 24, 2019
@apjanke apjanke deleted the support-jsonstuff branch December 24, 2019 12:23
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