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

Rename JSON to Value #473

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Rename JSON to Value #473

wants to merge 2 commits into from

Conversation

gfontenot
Copy link
Collaborator

We're going to be shifting the marketing focus for Argo away from being a
JSON parser and towards being a general purpose parser for transforming
loosely typed data models into strongly typed ones. As a part of this
change, we can generalize our types a bit, and use the name Value for our
input values, instead of JSON. This change is purely cosmetic, but will
result in a more general API, letting us break a bit from our ties to JSON.

Note that we're providing a typealias from JSON to Value here as well.
This should make this a non-breaking change (although users will get a
deprecation warning). I'd love it if other people could test this with their
codebase.

@@ -50,7 +50,7 @@ public func decode<T: Decodable>(_ object: Any) -> Decoded<T> where T == T.Decod
- returns: A `Decoded<[T]>` value where `T` is `Decodable`
*/
public func decode<T: Decodable>(_ object: Any) -> Decoded<[T]> where T == T.DecodedType {
return Array<T>.decode(JSON(object))
return Array<T>.decode(Value(object))

Choose a reason for hiding this comment

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

Syntactic Sugar Violation: Shorthand syntactic sugar should be used, i.e. [Int] instead of Array. (syntactic_sugar)

@mdiep
Copy link
Contributor

mdiep commented Oct 3, 2017

This should make this a non-breaking change (although users will get a
deprecation warning).

There's a caveat here: this isn't a source breaking change, but it is a binary breaking change.

@jshier
Copy link
Contributor

jshier commented Oct 3, 2017

It would be versioned, so how is that relevant?

@mdiep
Copy link
Contributor

mdiep commented Oct 3, 2017

We ran into an issue with this with ReactiveCocoa or ReactiveSwift. I can't find the specific GitHub issue, but @andersio or @ikesyo might remember how to find it.

IIRC, it becomes an issue if frameworks are compiled against different versions of Argo.

Imagine you have A.framework and B.framework. Both A and B have declared that they're compatible with Argo 4.1..<5.0. When you compile A, it uses Argo 4.1. When you compile B, it uses a copy of Argo, say 4.2, that includes this change. The compiled versions of A and B cannot be used together.

This will lead to build failures when using Carthage in many cases. (Depending on how A and B are set up.) I'm unsure whether this is a problem in practice with CocoaPods or SwiftPM. Semantic version unfortunately doesn't really address the difference between source compatibility and binary compatibility—that's part of the reason I don't think we should use semver.

@gfontenot
Copy link
Collaborator Author

This would only really be an issue if we didn't release this as 5.0, right? I was assuming that the rename changes would come with a 5.0 label because I do see binary breakage as being a breaking change.

@jshier
Copy link
Contributor

jshier commented Oct 3, 2017

@mdiep I'm not really sympathetic to that scenario. If you're not properly managing your versions, or you're distributing third party libraries precompiled, it's up to you, not the library, to ensure the versions work together. It's the same reason we don't distribute static Alamofire libraries: we don't want to be bound by whatever binary issues may ensue. To me it's only valid to consider the binary compatibility of libraries that are actually distributed as binaries. Especially until Swift has an actual ABI. At that point managing binary compatibility becomes much much easier.

@mdiep
Copy link
Contributor

mdiep commented Oct 3, 2017

This would only really be an issue if we didn't release this as 5.0, right? I was assuming that the rename changes would come with a 5.0 label because I do see binary breakage as being a breaking change.

Correct. I only brought it up since you mentioned this was a non-breaking change and this previously caught me be surprise. ☺️

If you're not properly managing your versions, or you're distributing third party libraries precompiled, it's up to you, not the library, to ensure the versions work together.

This isn't about distributing precompiled libraries. It's noting that "breaking change" according to SemVer doesn't distinguish between source-breaking and binary-breaking.

A common way this might arise: A framework includes its dependencies as submodules and builds against the submodule versions. These aren't precompiled. They're just a different version than the one you might ultimately link against.

@jshier
Copy link
Contributor

jshier commented Oct 3, 2017

Submodule integration, unless managed by a tool, falls under the "not properly managing your versions" umbrella. To my mind, libraries aren't responsible for version conflicts in a non-version managed environment. Frankly I think SemVer doesn't mention binary compatibility because it assumes ABI stability on the part of the language itself. Swift is rather unusual in that case and the concern will go away after Swift 5.

@mdiep
Copy link
Contributor

mdiep commented Oct 3, 2017

Submodule integration, unless managed by a tool, falls under the "not properly managing your versions" umbrella.

These submodules can be managed by Carthage.

Frankly I think SemVer doesn't mention binary compatibility because it assumes ABI stability on the
part of the language itself

I'm not sure those are the same thing. AIUI ABI stability guarantees compatibility between different versions of the compiler, which is not necessarily the same thing as guaranteeing that a source-compatible change is always a binary-compatible change. (As evidenced by the fact that this problem occurs when using the same version of swift for all dependencies.)

Frankly I think SemVer doesn't mention binary compatibility because it assumes ABI stability on the part of the language itself.

I think the more likely explanation is that Tom Preston-Werner (SemVer's author) is primarily experienced in Ruby and JavaScript. ☺️

@gfontenot
Copy link
Collaborator Author

7 months seems like it's long enough to wait for feedback on a change like this, right? Any last-minute objections?

@jshier
Copy link
Contributor

jshier commented Apr 25, 2018

Given that the Swift team seems to see no importance in being able to decode anything but Data, has thought been given to how this change, or Argo in general, could be used as a Codable wrapper for Any and such? Or is that planned already?

@gfontenot
Copy link
Collaborator Author

I don't think I've put much thought into Codable for Any at all, but I do think it's worth re-framing Argo as a more generic library for converting from weakly typed data structures into strongly typed ones. That sure sounds like what you're describing though.

@jshier
Copy link
Contributor

jshier commented Apr 25, 2018

Yes. There's been some discussion on the Swift forum regarding how to handle unstructured values in regards to decoding JSON (and like other decoding situations). Some users had created a JSON enum almost exactly like Argo, and I think someone had made Argo's JSON Decodable as well. It would be nice if the new Argo Value type could fill that niche OOTB.

Copy link

@sharplet sharplet left a comment

Choose a reason for hiding this comment

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

Just throwing this out there: what do you think about the name AnyValue? There's lots of precedent in Swift for weakly-typed values using the Any prefix (AnyObject, AnyKeyPath, AnyHashable, AnySequence). I kinda like the idea that AnyValue encodes a value with a dynamic structure.

@gfontenot
Copy link
Collaborator Author

Oh, I like AnyValue. My brain thinks of Any* types to be type erasers, though, where as this is more of a type... pencil? What's the dual of a type eraser? Identifier?

@gfontenot
Copy link
Collaborator Author

@jshier I dig the direction you're going, fwiw. I don't want to derail this conversation though. Want to open a new issue with a proposal (if you have one, or a basic discussion starter if you don't)? It seems like this might dovetail with #481, too.

@sharplet
Copy link

My brain thinks of Any* types to be type erasers, though

Yeah, I'm not sure AnyValue is quite right. Although I do think it would read nicely at the point of use:

static func decode(_ v: AnyValue) -> Decoded<Foo> {
  // ...
}

Things I like about it:

  • Communicates that the thing you have has a weaker type than the thing you want
  • Succinct
  • Idiomatic

Possible concerns:

  • It's not really a type eraser
  • Possibly implies that you can dynamic cast, which is true for things typed Any, AnyHashable and AnyKeyPath (but is not true for AnySequence or AnyCollection)

@jshier
Copy link
Contributor

jshier commented Apr 26, 2018

It's also not "Any" value but a limited subset of values. Even if the supported types expand beyond the current design, "Any" will always be a bit misleading.

@sharplet
Copy link

It's also not "Any" value but a limited subset of values.

IMO the idea is less that it could be any value, and more that it's a valid in-memory encoding of any value.

@gfontenot gfontenot force-pushed the gf-json-rename branch 2 times, most recently from e2e2aee to c7b6c93 Compare April 29, 2018 15:25
}
}


Choose a reason for hiding this comment

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

Vertical Whitespace Violation: Limit vertical whitespace to a single empty line. Currently 2. (vertical_whitespace)

We're going to be shifting the marketing focus for Argo away from being a JSON
parser and towards being a general purpose parser for transforming loosely
typed data models into strongly typed ones. As a part of this change, we can
generalize our types a bit, and use the name `Value` for our input values,
instead of `JSON`. This change is purely cosmetic, but will result in a more
general API, letting us break a bit from our ties to JSON.
This should make the JSON -> Value rename a non-breaking one. Existing code
using the JSON type will continue to work, but will also prompt users to
switch to the Value type.
@mdiep
Copy link
Contributor

mdiep commented Apr 29, 2018

Would it make sense to be generic over some Value type instead?

JSON really is JSON-specific. You might want to work with a different set of primitive types. e.g. if you wanted to work with a plist, you might add date and data and rename object to dictionary. JSON could be left as-is to support JSON out-of-the-box.

That would also require adding format-specific Decodable protocols, which is a little bit of a pain, but not too bad.

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

5 participants