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 cases to support casting from json.Number values. #61

Closed
wants to merge 1 commit into from

Conversation

ottodashadow
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2018

CLA assistant check
All committers have signed the CLA.

@yanmhlv
Copy link

yanmhlv commented Sep 18, 2018

Hi @bep ! Could you please merge this PR?

Copy link
Collaborator

@bep bep left a comment

Choose a reason for hiding this comment

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

Instead of doing this for json.Number (which is both limiting, and it add a package dependency -- if you run GopherJS you might care about that, but that is not my main argument).

Instead I would suggest you add some internal interfaces with Int64() and check for those interface types in the case switches.

if err == nil {
return float32(v), nil
}
return 0, fmt.Errorf("unable to cast %#v of type %T to float32", i, i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suspect that a "fallthrough" here would have the same effect (and be shorter).

Copy link

Choose a reason for hiding this comment

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

https://github.com/golang/go/wiki/Switch#fall-through

Note: fallthrough does not work in type switch.

@bep
Copy link
Collaborator

bep commented Mar 18, 2019

@yanmhlv @ottodashadow can this be closed or do you plan to work on this?

@yanmhlv
Copy link

yanmhlv commented Mar 19, 2019

@bep ok, I'll fix it in my own PR

bep pushed a commit to bep/cast that referenced this pull request May 11, 2022
@bep bep closed this in 408043c May 11, 2022
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