Improvements and fixes for Stream library.

[Chris--A]

This set of commits improves the parsing capabilities of Stream::parseFloat() and Stream::parseInt().

parseFloat

The function will now accept decimals starting with an . character not just 0.

An error has been fixed where where 1.23.45 is parsed as 1.2345 rather than 1.23 and subsequent calls reading _0.45_ (after the fix mentioned above).

both parseInt/Float

The lookahead feature can be controlled using either SKIP_NONE, SKIP_ALL, or SKIP_WHITESPACE

The hidden member skipChar has been exposed. This allows parsing of formatted decimals, not just shorthand like .12 but complex formatting like: -1,234.567 (parseFloat), 1,234 (parseInt). Rather than removing the hidden unused features, they could be quite beneficial.

As the interface changes are implemented using optional parameters, the code is backwards compatible without change.

SAM

The Stream::find(char); method has been added to the SAM core making it equivalent to the AVR implementation.

This resolves:
#1962
#2007
#630

[AbstractBeliefs]

👍

[matthijskooijman]

I've quickly glanced over the code, and it seems ok (haven't looked close enough to really check it, though).

Regarding the commit itself, I'd rather see this as two separate changes, since AFAICS they are separate both in function as well as in implementation? Care to split the commit in two? Regarding the commit message, it would be better to remove the "this patch" part, since the message describes a commit, not a patch. In general, a direct form is shorter and preferred, so "Improve the parsing capabilities..." (or if you split the patches, the message can be more specific: "Make parseFloat stop parsing at a second decimal separator" , or "Make parseFloat work without a leading zero", something like that.

[AbstractBeliefs]

@matthijskooijman it does have to be said, this does exist as two separate changes that can be used (with a bit of TLC), however, they've both been left to rot.

I understand that many of the people who can pull these requests are volunteers (but the same can be said of other projects) and Arduino has a particular reputation of being... inconsistent in which pull requests are/aren't accepted and the quality of feedback in either case.

Why not just apply the original PRs, which have already been set up and reviewed?

[matthijskooijman]

@AbstractBeliefs, I can't comment on why / when PR's are (not) merged. I'm not in a position to make such decisions. I just try to give feedback on contributions, to get them in the best possible shape and make their merging as easy as possible for the people that do the merging. Also, there might be some discrepancy in my feedback and the de-facto requirements for a commit / PR, I'm known to have very high standards in terms of history tidyness :-)

[Chris--A]

@matthijskooijman, no worries, I will change it up a bit.

[Chris--A]

@matthijskooijman
I have re-based the initial commit, separating it into two specific changes.

@facchinm
You added the Stream::find(char); to the AVR core only in ed1b8eb I have added the SAM version in: Chris--A@94007e2, (#847)

I have added in a modification which resolves #630 and I have also exposed some previously hidden functionality that can be quite useful. Checkout the original PR message for a revised overview of the commits.

[matthijskooijman]

PR looks better now. I had a closer look at the actual code too, and added some comments inline.

Regarding the commit messages, I still have one more request. The style commonly used is:

• A first, short (<60 chars preferably) line describing the essence of the commit. For brevity, this line shouldn't have extra fluff like "this commit makes" (instead just say "make"). This line should describe what is changed, not why it is changed, what bug number is fixed, etc. Also, this line shouldn't have a trailing period.
• An empty line.
• A more detailed commit message, describing the change in more details. This should include rationale for the change, any caveats, side-effects, etc. Using "This commit" in this part of the message is ok. Sentences should use trailing periods.
• If a bug is closed by the commit, it should have "Closes #000" in the commit message to allow github to automatically close the commit. If a commit is related to a bug but doesn't close it, just use the bugnumber in the commit message somewhere (e.g. "This commit prepares for fixing #000.").

These are not hard-and-fast rules, but ideally all commits follow these guidelines. We should probably put them up in some good place and have github reference them when creating a PR....

[AbstractBeliefs]

As an additional comment about documentation, the functions exposing more controls (such as skipChar and skipMode) should have their documentation updated here. Currently there's no way for the general public to do that (sadly). How should we trigger this once the changes are pulled in?

[matthijskooijman]

Updating documentation is usually handled by opening up an issue, preferably already containing some suggested text for the documentation, so the Arduino team can copy that into the online docs.

[Chris--A]

I have finalized this PR and it now is ready for a final review. :)

I made a decision in the last commit to keep some overloads protected. This is to keep the public API simple. I would prefer these to be public, however I did what I thought would be better for newbies. What do you think?

[matthijskooijman]

Two more remarks:

• When adding the skipMode parameter to peekNextDigit(), why not add it as a second parameter with a default, to keep backward compatibility? AFAICS, this would make the commits a lot easier to read (I might be wrong, but especially the last commit is a bit big and I don't have a lot of time right now).
• The commit adding comments should ideally be squashed into the previous commit where the constants were introduced. I can't see any reason for this to be two commits?

[Chris--A]

@matthijskooijman, I had an absolute nightmare removing the comments from the unrelated commit previously, so I left it as its own commit. However I have managed to rebase it today into the previous commit.

As for peekNextDigit() the first parameter was added in this PR's first commit: Chris--A@2e12a79 so it will not break any existing code.

[matthijskooijman]

Ah, I see what you mean now and indeed, no existing code will be broken. However, the history is a bit messy and confusing by this. Ideally, the commits would be reordered such that the protected members are made public first, and then adds skipMode as a second parameter. AFAICS that would result in a much cleaner patch (which is good for reviewing and for the history). Does that sound reasonable to you?

[Chris--A]

Initially the user/public interface had a parseInt() & parseFloat() accepting no parameters. Only derived classes had access to parseInt(skipChar) & parseFloat(skipChar). I intentionally introduced the skipMode enum as the first parameter to peekNextDigit() as it reflects how it is used with the parsing functions (it was the only parameter in the public API), and this function is private so it is an internal change only.

Exposing the protected features to the public interface is unrelated to the addition of the skipMode functionality, and re-ordering would only impact the ordering of the parameters skipMode and skipChar, where it would have replaced skipChar as the first parameter.

The history may look slightly better if I did not remove the protected member and replace it later. However there is a comment why the function is protected: Chris--A@8cead55#diff-55eb81fc7b42d6d266a05aee3b593ed3R114

Having skipMode as the first parameter in parseInt/parseFloat (in my opinion) is keeping in tune with common orderings like coarse grained to fine grained/most used to least used. Of course the single parameter version of parseInt(skipChar) & parseFloat(skipChar) can co-exist in the public API (there is no ambiguity), I just left it protected to avoid confusion.

[matthijskooijman]

Hm, seems part of my confusion stems from the fact that 'skipChar' is erronously documented as a public interface, probably because it was actually public before 1.0.

Regarding the preferred ordering, I'm not so sure if either skipChar or skipMode is really differently grained and I can't really predict what would be more used, but I don't have a strong preference for the order either way, I think.

Of course the single parameter version of parseInt(skipChar) & parseFloat(skipChar) can co-exist in the public API (there is no ambiguity), I just left it protected to avoid confusion.

Right, that's probably the confusion that hit me too :-p An advantage of swapping the parameters is that there is no need for a separate single-parameter version, which I think is what I was aiming for.

Anyway, if we stick to the parameter order you suggest (as I said, I don't have a strong preference), then I think that the first commit (that introduces the parseX(skipMode) public method, should not remove the protected parseX(skipChar) method, but instead add a protected parseX(skipMode, skipChar) alongside it, which is called by both single-parameter versions. This prevents the first commit from breaking the API, and will make the second commit significantly simpler - it will just make a protected method public as its commit messages promises. And, re-reading your comment, this is exactly what you meant saying "if I did not remove the protected member and replace it later.". I would agree that the history would look cleaner, though I can imagine you're tired of my nitpicking already :-p

On an unrelated note, thinking about the grained-ness of the parameters made me realize that one of them is about skipping stuff before the number and one of them is about skipping stuff inside the number. Would it make sense to change the parameter names to reflect this? If you have a brief look at the names now, you might think that skipChar is a character to skip and skipMode indicates in what way it should be skipped. Perhaps skipStartMode and skipInsideChar might be better?

[Chris--A]

I'll fix up the ordering a bit,

For the parameters, maybe: lookahead & ignore. I'll change the enum name to LookaheadMode.

[Chris--A]

Ok, the breaking changes that were added and removed are now gone from the history. It can now be reverted to any commit without breaking backwards compatibility.

I have renamed the parameters and enum as discussed above 🌴

EDIT: if merged, I'll create a PR to update the zero core.

[matthijskooijman]

@buildbot, build this please.

Code looks good to me now!

Two more remarks about the commit messages:

• The first commit has "This adds the capability to control the Stream::parseInt/float" as the summary line, which is an incomplete sentence (which continues in the rest of the commit description on subsequent lines). It is customary to make the first line self-contained, so something like "Allow controlling Stream::parseInt/Float lookahead" is probably better there, using the entire current commit message as the detailed description after the empty line.

  Also, it would be good to mention the skipChar -> ignore rename in the commit message, that makes the patch easier to review.

• The subject line of the last commit, "Enables public access to features previously marked private" is very general. I would propose changing it to e.g. "Make protected Stream::parseInt/Float overloads public".

  The rest of the commit message is fine.

[Chris--A]

The commit messages have been edited.

Chris--A@004838a now mentions the changes skipChar -> ignore.

[Chris--A]

Anything holding this back from being merged?
As soon as this is approved (if), I will also do up a PR for the zero (and third party ESP8266) to keep a contiguous API.

I hope these improvements do not get lost in the abyss.

I can also rebase the commits if needed so it doesn't throw out the history.

[matthijskooijman]

+1 for merging this.

[AbstractBeliefs]

I'd like to see this merged also.

[BlackBrix]

why isn't this merged, it is needed ...

[DaveAhrendt]

Same here. Please merge this.

[sandeepmistry]

Thanks @Chris--A! I've rebased your PR and pushed it to master. It will be available in the next hourly build: http://www.arduino.cc/en/Main/Software#hourly

@matthijskooijman, @AbstractBeliefs thank you for reviewing and providing feedback as well.

[matthijskooijman]

Now that this is merged, the documentation should be adapted. @agdl, could you take a look? @Chris--A, perhaps you could propose changes and additions to the reference documentation so @agdl can use them directly?