refactor: Remove usage of fmt in library #72 #81
Conversation
do the string concatenation manually
bugfix: add the string header
the exact type of current.id is unknown, try using a generic method to convert this value into a string.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 23 23
Lines 1226 1234 +8
=======================================
+ Hits 1222 1230 +8
Misses 4 4
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @MichaeINeumann, thanks for working on this! Don't worry about being new to C++, I think this is a nice introductory ticket.
I see you found a fix for the failing tests on your branch, congrats 🎉 In short I think both the approaches are valid.
The std::to_string approach relies on the current locale for formatting options (see cppref for more details). This works, but formatting of floats and doubles is not consistent across clients.
Since the tests in shortest_path_test.cpp are expecting error messages created with fmt::format, this results in failing assertions such as "Negative edge weight [-1.000000] between..." == "Negative edge weight [-1] between...". A fix would be to not create the expected error message using fmt, but rather use the same approach used as generating the error message (std::to_string(weight_t{-1})).
Using sstream is another approach, which is closer (if not equal) to what fmt uses under the hood. So with this approach the tests can keep using fmt.
I slightly favor the current approach as it allows us to use fmt in the test executables, making testing a bit more developer friendly.
|
LGTM, congrats on the first real code contribution 🚀 |
do the string concatenation manually Issue:#72
@bobluppes @joweich I created my own PR to see if the code would pass the test. I am new here and have no experience. I don't know if this is the right approach in this case. Perhaps you should delete this duplicate PR. #75