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

Support parsing XMLs with integers in bases different than 10 #19

Merged
merged 2 commits into from
Oct 22, 2016

Conversation

KenjiTakahashi
Copy link
Contributor

Some folks over at NetBSD like their XMLs with hex encoded integers. Since strconv parsers can derive base on their own, I think it will be best to let them do it.

@dmitshur
Copy link

dmitshur commented Aug 28, 2016

@DHowett any review comments on this PR? Should it be accepted or rejected? What would it take to make progress here?

I'm not very familiar with go-plist and its details, but this change seems reasonable (and backwards compatible) to me.

@DHowett
Copy link
Owner

DHowett commented Aug 31, 2016

Interesting. This change absolutely makes sense.

On OS X, hex int literals in plists work fine, but octal ones are ignored (treated as decimal). I'm willing to break with compatibility to support octal literals, if there's a compelling reason.

If not, we should retool this so that it only switches to base 16 with the 0x prefix.

@KenjiTakahashi
Copy link
Contributor Author

I don't have any reason for octal numbers support, other than it being easier to implement. I need only hex.
If you wish to stick to what OS X does, I'd adjust this change to only threat hex numbers specially.

@DHowett
Copy link
Owner

DHowett commented Sep 9, 2016

That would be great! Let's stick with what OS X does.

@KenjiTakahashi
Copy link
Contributor Author

Sorry for such a long delay, real life has caught up with me :-). Updated now.

@DHowett
Copy link
Owner

DHowett commented Oct 6, 2016

This looks great. Would you mind throwing in at least one positive and one negative decode test?

@KenjiTakahashi
Copy link
Contributor Author

@DHowett Updated. Let me know if there's anything else.

@DHowett DHowett merged commit fd61394 into DHowett:master Oct 22, 2016
@DHowett
Copy link
Owner

DHowett commented Oct 22, 2016

Thanks a lot for contributing, and waiting, and putting up with my change requests!

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