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

Add automatic type conversion #68

Merged
merged 15 commits into from Mar 22, 2016
Merged

Conversation

ncreated
Copy link
Contributor

This PR adds automatic type conversion, like this:

let books: [Book] = try xml["root"]["books"]["book"].value()

Inspired by discussion in #10

@drmohundro
Copy link
Owner

This looks great! I just have a few thoughts...

  • Any thoughts on the name of value()?
    • My primary concern is that, if someone is new to the project and sees Xcode show .value() as an option, they might try it and then see it fail because they didn't implement XMLIndexerDeserializable.
    • The counterpoint is that, for most of the built-in types .value() should work just fine.

Given that it should just do the right thing if the code indexes down to a built-in type, .value() is probably actually better. So I think I just changed my own mind!

  • Any thoughts on adding a built-in Bool deserializer? We've got String, Int, and Double.
    • I have concerns about this as well because there would be variations like "yes", "no", "true", "false", "0", "1", etc.
    • It might be better to not ship that, at least not yet. I'd be fine holding off on that to see if people request it.

I'm fine merging in as-is, but I wanted to see if you had any thoughts related to my questions above.

@ncreated
Copy link
Contributor Author

I added basic support for Float type (483cbe6).

As for the Bool - W3C says that legal literals are true, false, 1 and 0, so we might start with this. I'm also OK with adding it later, on people's request 👍. Btw, being strict with W3C guidelines for float and double might be a separate improvement.

If it comes to .value() - I had no better idea for that name. I tried .convert() for a while, but it doesn't have a getter meaning.

And you're right with the concerns of people being mislead about using .value() on types that don't implement XMLIndexerDeserializable. Swift compiler doesn't help with this hint:

zrzut ekranu 2016-03-22 o 01 22 19

I think that including possible explanation of "Ambiguous reference to member 'subscript'" error in README.md might be helpful. WDYT?

@drmohundro
Copy link
Owner

+1 on Float. Also, let's hold off on the Bool converter for now.

We're in agreement regarding adding an explanation or something about the "ambiguous reference" error - I've been thinking about adding some type of FAQ.md or DebuggingTips.md or something like that, so it might be a good candidate for something like that. Don't worry about that for now.

This looks good to me! If you're ready, I'll go ahead and merge and start getting this packaged up for a new release for CocoaPods and Carthage.

@ncreated
Copy link
Contributor Author

@drmohundro I'm happy with how it looks now. Can be merged 💥. Glad to hear that you like it 😉.

I'd rather vote for adding FAQ section in README.md since README.md is the first place where I do ⌘+F in case of strange errors. But it might be only me 😃.

drmohundro added a commit that referenced this pull request Mar 22, 2016
@drmohundro drmohundro merged commit ec763a3 into drmohundro:master Mar 22, 2016
@gca3020
Copy link
Contributor

gca3020 commented Apr 7, 2016

Not sure if this is the right place to comment, but this functionality is fantastic. I have some pretty complicated classes that I'm filling in from XML, and this looks like it will help with that quite a bit. I'm new-ish to Swift, and this beats the pants off of the complicated failable initializers and mountains of guard statements I started prototyping a while back.

You mentioned above adding a Boolean deserializer, and I'd like to throw my hat in the ring for wanting that feature. I have some XML now that uses "1" and "0" for true and false, but as you mentioned, I've also seen "true" and "false", or "yes" and "no" (occasionally even "on" or "off", and "enable" or "disable", but that was very non-standard).

I can't imagine too many people complaining about adding deserializers for "1"/"true" and "0"/"false". That seems like it would cover the vast majority of common cases.

@drmohundro
Copy link
Owner

@gca3020 thanks for the feedback! I'm up for it - I'll see if I can get around to it in the next day or two.

@ncreated ncreated deleted the type-conversion branch April 7, 2016 07:11
@ncreated
Copy link
Contributor Author

ncreated commented Apr 7, 2016

@gca3020 thank you, I hope you'll find it helpful. @drmohundro I can also look into it in next few days, so let me know if I can help. I guess, we should open new Issue with this request, but leave it for you.

@gca3020
Copy link
Contributor

gca3020 commented Apr 7, 2016

@drmohundro @ncreated Actually, looking at it, I'd love to use this opportunity to get a little more familiar with the GitHub Pull Request process, and contribute this functionality myself. I'll go ahead and open an issue and see if I can't get the code and everything done this weekend.

@drmohundro
Copy link
Owner

@gca3020 go for it - and we can move the discussion to #70. I'm happy to help out!

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