Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base(844e4bf) 95.70% compared to head(0578e7e) 95.57%.
Report is 76 commits behind head on master.

@@            Coverage Diff             @@##           master     #181      +/-   ##==========================================- Coverage   95.70%   95.57%   -0.14%==========================================  Files          13       16       +3       Lines         954     1084     +130     ==========================================+ Hits          913     1036     +123- Misses         41       48       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

Hi@jfjlaros ☕ 👋

Thank you for your contribution. It should still be comparatively easy to add test code within thetest subfolder.

Frankly, I'm not going to approve the API in its current state, makePair just sounds way to similar to std::pair, but has a completely different effect. That's going to throw people for sure.

I will have a look at the unit tests.

makePair just sounds way to similar to std::pair

Hmm.makePair is supposed to be analogous tostd::make_pair, the container is namedPair (analogous tostd::pair).

I could add a second constructor toPair to allow for syntax like this:

Serial << Pair(1.2,4) <<'' << Pair(12, BIN) <<"\r\n";

then the functionmakePair could be dropped if that is what you prefer.

Hmm. makePair is supposed to be analogous to std::make_pair, the container is named Pair (analogous to std::pair).

Well yes. So why re-inventstd::pair? Maybe useformat or similar. Something that tells the user what's about to happen.

why re-inventstd::pair?

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

I could make something more specialised I guess.

The unit tests seem to fail because of a failed upload in the workflow. Perhaps someone could re-trigger it.

Thanks@per1234.

@aentinger, I followed your suggestions and updated the PR. I guess now the description should be as follows.

In this PR, the following is added:

• Format: a container for one arbitrary object and one modifier of typeint.
• format(): a helper function to create aFormat instance.
• << a basicStream insertion operator for compact and convenient writing to aStream.
• << an overload to allow for modifiers likeBIN,HEX and the number of digits to be written.

Usage:

int a {9};int b {10};Serial <<"You have" << a <<" out of" << b <<" retries left.\r\n";// Prints: "You have 9 out of 10 retries left."Serial << format(1.2,4) <<'' << format(12, BIN) <<"\r\n";// Prints: "1.2000 1100"

Hmm, same problem with the failed upload again...

I apologize for the inconvenience@jfjlaros. Unfortunately these spurious failures caused by transient network faults are a common occurrence when using GitHub Actions.

I reran the job and it passed this time.

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

std::pair is available on some Arduino platforms and by creating something so similar, you violate the expectations of all developers when you create something very similarly named but doing something different. This shall be the last time I discuss this, and you've already adjusted as I can see.

Because the standard template library is not available, or is this not the case for cores that depend on this repository?

I'm aware that at least AVR does not support it, I think most if not all other cores do have it. For AVR, this might be solveable by enabling libstdc++, seearduino/toolchain-avr#89 about this (this is off-topic for this PR, but I wanted to mention it here in case it is interesting to anyone here).

this might be solveable by enabling libstdc++

That would be great, I have my own implementations ofstd::vector,std::tuple andstd::enable_if that I would happily part with.

Perhaps the "Changes requested" block can be resolved (as all the underlying requests have been addressed)?

Has anyone considered the impact of a global scope function named "format" and a global scope class named "Format" on the existing Arduino ecosystem? Will these cause some programs and libraries to break?

Will these cause some programs and libraries to break?

Probably, I find it hard to tell.

We could simply rename them or not introduce them into the global scope at all. I would prefer the latter.

I have removed both theFormat class as well as theformat function from the global scope. I hope this alleviates any remaining concerns.

• format(): a helper function to create aFormat instance.

Can it return aString?