Skip to content
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.

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

Remove std::endl from print functions #2018

Open
p-zach opened this issue Feb 5, 2025 · 6 comments
Open

Remove std::endl from print functions #2018

p-zach opened this issue Feb 5, 2025 · 6 comments

Comments

@p-zach
Copy link
Member

p-zach commented Feb 5, 2025

Issue

Currently, classes append std::endl in their print statements, such as Pose2:

void Pose2::print(const string& s) const {
  std::cout << (s.empty() ? s : s + " ") << *this << std::endl;
}

It seems to me that this is unnecessary and goes against best practice. It means that users have less control over how they can format their print statements. For example, I would have liked the Python code

print(f"{global_point} transformed by {origin} into local space -> {transformed}")` 

to yield

[5. 5.] transformed by (1, 1, 3.14159) into local space -> [-4. -4.]

but instead it gives the following:

Image

This example also demonstrates best practice as followed by Numpy, which does not automatically insert newlines.

Solution

Remove std::endl from print functions. Perhaps it can be implemented so that the endline is only added if the user has not supplied a prefix string. I can create a PR if this solution is desired.

Caveats

This is an opinionated issue, but I think that allowing the user to format their strings how they wish is strictly better than the current approach. The optional string prefix is handy for debugging and writing quick statements, but for even slightly more elaborate formatting it gets in the way.

@dellaert
Copy link
Member

dellaert commented Feb 5, 2025

I sympathize, but I don't want to break existing workflows on current behavior. However, there is a solution: phase out print (in a few releases) in favor of streaming. Some classes already support ostream, with no prefix or endl. If you wanted to make a PR with that (and where you can use it in print, to avoid duplication), that would be a very worthwhile contribution. Could be done one subfolder at a time, e.g, starting with geometry.

@dellaert
Copy link
Member

dellaert commented Feb 5, 2025

PS how to surface that in python to support {} would have to be looked into; the wrapper should maybe hook up to streaming rather than print.

@p-zach
Copy link
Member Author

p-zach commented Feb 5, 2025

I see what you mean. For future reference, I think the place to modify would be

ret += '''{prefix}.def("__repr__",

@varunagrawal
Copy link
Collaborator

PS how to surface that in python to support {} would have to be looked into; the wrapper should maybe hook up to streaming rather than print.

This is easy to do in the wrapper once we have full streaming support. :)

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 8, 2025

The current python printing is a shortcut to just wrap the old print() functions. One probable next step is moving all print methods to proper formatters with fmt (I have prototyped this a few years ago) and rewrite the existing print() functions to just call the fmt formatter.

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 8, 2025

Furthermore, we can (and should!) support the custom formatting in Python with "{:.2f}".format(pose3) style by overloading the __format__(self, format_str) method.

BTW the fmt::format(fmtlib) or std::format (C++23) style of logging/printing has been everywhere now. But ostream is still much much better than print.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants