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 the test 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 to std::make_pair, the container is named Pair (analogous to std::pair).

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

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

then the function makePair 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-invent std::pair? Maybe use format or similar. Something that tells the user what's about to happen.

why re-invent std::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 type int.
• format(): a helper function to create a Format instance.
• << a basic Stream insertion operator for compact and convenient writing to a Stream.
• << an overload to allow for modifiers like BIN, 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++, see arduino/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 of std::vector, std::tuple and std::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 the Format class as well as the format function from the global scope. I hope this alleviates any remaining concerns.

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

Can it return a String?