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

change ordered keyword to dicttype keyword #129

Merged
merged 1 commit into from
Oct 5, 2015
Merged

change ordered keyword to dicttype keyword #129

merged 1 commit into from
Oct 5, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Oct 5, 2015

As discussed in #128, this provides more flexibility, and avoids the pseudo-dependency on DataStructures that has been causing precompilation problems.

@tkelman
Copy link
Contributor

tkelman commented Oct 5, 2015

lgtm. gotta love when adding generality fixes more problems than it introduces. worth changing minor pkg version for this, even if apparently unused in public code?

JSON.parse(io::IO; ordered=false)
JSON.parsefile(filename::AbstractString; ordered=false, use_mmap=true)
JSON.parse(s::AbstractString; dicttype=Dict)
JSON.parse(io::IO, dicttype=Dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

, should be ;?

Copy link
Member Author

Choose a reason for hiding this comment

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

right, fixed

@IainNZ
Copy link
Contributor

IainNZ commented Oct 5, 2015

So very clean!

…ility, and avoids the pseudo-dependency on DataStructures that was causing precompile problems
stevengj added a commit that referenced this pull request Oct 5, 2015
change ordered keyword to dicttype keyword
@stevengj stevengj merged commit eb65870 into master Oct 5, 2015
@stevengj stevengj deleted the ordered2 branch October 5, 2015 13:02
@kmsquire
Copy link
Contributor

kmsquire commented Oct 5, 2015

While I like this change, I've seen the ordered keyword used in the wild, so the next tag of JSON.jl is going to break some code. Any thoughts on deprecation or informing the user how to fix her code?

@stevengj
Copy link
Member Author

stevengj commented Oct 5, 2015

@kmsquire, I couldn't find it in any registered packages.

But one could certainly add an ordered=false keyword that throws an informative exception if it is changed to ordered==true.

@kmsquire
Copy link
Contributor

kmsquire commented Oct 5, 2015

Thanks, yes, I mentioned that this was in user code. I'll add the exception when I get the chance.

@tkelman
Copy link
Contributor

tkelman commented Oct 5, 2015

sorry for the hasty tag, should have cc'ed you sooner on this

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.

4 participants