Skip to content

Fix for Issue #1962 #2007

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix for Issue #1962 #2007

wants to merge 2 commits into from

Conversation

ChocoChipset
Copy link

Pull request to solve issue #1962.

Floating point numbers with a trailing . character would be rendered as integers.
For example: .032 => will become 32.

The new function peekNextDigitOrPoint takes into account this and is used in the function parseToFloat.

Let me know if any re-factor or modification is required.

@matthijskooijman
Copy link
Collaborator

It looks like a correct fix for the issue, thanks. I do have a few remarks:

  • The commit message is a bit short. Preferably a commit message has short first line the summarizes what changed (e.g., "Allow a leading decimal separator in Stream::parseFloat"), then an empty line and then more lines with detailed explanations. A reference to the github issue definately belongs in the commit message, but not in the first line. Also, of you write "Fixes Serial.parseFloat doesn't parse leading decimal points #1962", github can automatically close the issue for us.
  • You're now duplicating a bunch of code. It might be better to instead add a bool allowPoint argument to peekNextDigit.
  • While you're here, it might be good to rename peekNexDigit to something like skipUntilDigit, since the current name really doesn't describe what the function does. This should preferably happen in a separate commit, probably before the main fix.

@ChocoChipset
Copy link
Author

Thanks for all the pointers!

I agree on all remarks and will be resubmitting the pull request in brief.

@ChocoChipset
Renamed Stream::peekNextDigit to Stream::skipUntilDigit for clarity
d57ab34
@ChocoChipset
Allow leading decimal separator in Stream::parseFloat.
7eeeb82 
Fixes arduino#1962 :
Floating point numbers with a trailing '.' character would be rendered as integers.
For example: .032 => will become 32.
@ChocoChipset
Copy link
Author

Ready now, @matthijskooijman! ;]

@matthijskooijman
Copy link
Collaborator

Looks good to me!

Now, it's up to the dev team to merge.

@ChocoChipset
Copy link
Author

Purrfect!

@cmaglie cmaglie added Component: Core Related to the code for the standard Arduino API Type: Bug labels Apr 15, 2015
@Chris--A Chris--A mentioned this pull request Jun 15, 2015
Closed
@Chris--A
Copy link
Contributor

This PR is incomplete.

The changes require Stream.h to be updated also. I have a new PR which fixes this: #3337

@facchinm
Copy link
Member

Superseded by #3337

@facchinm facchinm closed this Jul 13, 2015
@ffissore ffissore modified the milestone: Release 1.6.6 Aug 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cmaglie cmaglie

Labels
Component: Core Related to the code for the standard Arduino API Type: Bug
Projects
None yet
Milestone
Release 1.6.6
Development

Successfully merging this pull request may close these issues.
6 participants
@ChocoChipset @matthijskooijman @Chris--A @facchinm @cmaglie @ffissore