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

Mix~in for local file #148

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Conversation

steve21124
Copy link

Fixes #147

test using these json
https://jasonbase.com/things/nz7.json => copy as main.json
https://jasonbase.com/things/3bA.json => copy as item.json
in setting.plist, set url to file://main.json

@gliechtenstein
Copy link
Contributor

Awesome, thanks for the pull request! I was taking a look and had a question:

https://github.com/Jasonette/JASONETTE-iOS/pull/148/files#diff-f22c800ecb5b9ecf974c06bdcb66ad7cR94 <= Was there a reason why you used VC.original = @{@"$jason": res[@"$jason"]}; instead of directly using VC.original = res;?

With mixins you can include any key in your JSON objects, so unless I'm missing something I think this may break if you try to mix in a local JSON object that has no $jason key or has other keys as well. Let me know!

@steve21124
Copy link
Author

Good catch. :). That section of code exactly the same as :

VC.original = @{@"$jason": res[@"$jason"]};

I think since on above "Reload" will always has $jason, so that's why it is there. But in here, there is case where it does not has $jason. Feel free to modify it. Or only the pull request person can modify it?

@gliechtenstein gliechtenstein merged commit b77924e into Jasonette:develop Mar 21, 2017
@gliechtenstein
Copy link
Contributor

@steve21124 sorry for the late merge! For some reason I actually thought I had merged it, just to find out a minute ago that I didn't.

Actually this has come to my attention because someone asked me if this was possible, which i thought it was, just to find out it's not yet merged. Anyway this means your feature already has a user, congrats, and thanks for the PR! :D

p.s. After a little more digging into code I think we're good to go with using the $jason unless there's something I'm missing. So no need to update the code!

@steve21124
Copy link
Author

np. Either way, I have not have a chance to check it out yet. It is too sunny in BayArea. not enough time to do side project over the weekend.

@gliechtenstein
Copy link
Contributor

@steve21124 haha enjoy!

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.

None yet

3 participants