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

parse double and floats #985

Closed
wants to merge 1 commit into from

Conversation

leon
Copy link

@leon leon commented Oct 18, 2017

When type is xs:double or xs:float we use parseFloat to convert it to javascript.

I'm unsure why this hasn't been done.
Maybe you have a good reason as to why you are not converting floats or doubles.
But since usually people want the data in their native form, I thought I'd suggest this atleast.

Tests:
There were 28 failing tests in master, so I was unable to write any tests.
I'd gladly write tests once master is passing again.

When type is xs:double or xs:float we use parseFloat to convert it to javascript.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 86.965% when pulling dd07a48 on leon:feature/parse-double-and-float into 24e4b30 on vpulim:master.

@coveralls
Copy link

coveralls commented Oct 18, 2017

Coverage Status

Coverage increased (+6.6%) to 93.606% when pulling dd07a48 on leon:feature/parse-double-and-float into 24e4b30 on vpulim:master.

@herom
Copy link
Contributor

herom commented Oct 18, 2017

thanks a lot @leon - it seems that #979 broke our build when it was merged in... somehow Travis CI/Coveralls wasn't running 🤕

I hope we get this fixed very soon - seems like #984 is a follow up to the breaking PR which will fix these issues.... I'll notify you once we're up and running again 💯

@jsdevel
Copy link
Collaborator

jsdevel commented Oct 18, 2017

@leon I've updated master with a fix for the bug introduced from #979. Can you rebase and update your branch?

@herom
Copy link
Contributor

herom commented Oct 19, 2017

@leon I restarted Travis CI and all current tests pass - could you please add tests for your PR? 👍

@leon
Copy link
Author

leon commented Oct 19, 2017

Where do we test all the other serializations?
I couldn't find where we test all the simple types.

@mattilindell
Copy link

Could you also add parsing of xs:long and xs:decimal ?

@alexb-uk
Copy link
Contributor

I've just come across this issue as didn't realise that type "double" wasn't being converted.

Can I help get this PR merged? Given how old this PR is I'm guessing @leon has probably moved on to something else?

Do you want me to submit a new PR with some tests?

@alexb-uk
Copy link
Contributor

Having said that I think it's safer to convert using Number(text).

@jsdevel
Copy link
Collaborator

jsdevel commented May 11, 2019

need a test added and conflicts resolved

@jsdevel jsdevel closed this Sep 19, 2019
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.

6 participants