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

Add ability to redirect all prints #75

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ShaharHD
Copy link

@ShaharHD ShaharHD commented Dec 4, 2024

Added output (which defaults to std::cout) and error (which defaults to std::cerr) to allow easier redirect of all prints.

Useful when used in a daemon for example or when using a more complex system like spdlog

@FlorianReimold
Copy link
Member

FlorianReimold commented Dec 11, 2024

This looks really good. Awesome technique to just pass a stream. I always thought about the best technique for redirectable log output and this may be it.
I will take a deeper look after the holidays and merge this PR 👍

*/
FINEFTP_EXPORT FtpServer(const std::string& address, uint16_t port = 21);
FINEFTP_EXPORT FtpServer(const std::string& address, const uint16_t port = 21, std::ostream& output = std::cout, std::ostream& error = std::cerr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

port should not be const, as it is passed by value (will give warnings otherwise)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue reverting it, but I never saw an issue with a const for any C or C++ int types when by-value is used (by-ref might be an issue)
(I try to use const on every param method to avoid typos or incorrect assignment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of where you might think it will fail?

@ShaharHD
Copy link
Author

ShaharHD commented Dec 12, 2024

This looks really good. Awesome technique to just pass a stream. I always thought about the best technique for redirectable log output and this may be it. I will take a deeper look after the holidays and merge this PR 👍

Thank you :) but you did 99.9% of the work with this awesome library 🥇

I see it it failing on some windows builds in the CI/CD and pushed a fix - please trigger the workflow again.

@ShaharHD
Copy link
Author

ShaharHD commented Dec 13, 2024

@FlorianReimold I've used CMake FetchContent_Declare with PATCH_COMMAND to test this in my code base - hence the commits to of fixing some additional issues.

Let me know if you prefer I squash those

Also, can you please run clang-tidy and push?
I've tried to match your code style, but I'm more of a clang-format guy ...

@FlorianReimold
Copy link
Member

I let clang tidy check the code. There were no additional warnings, except from the consts. I fixed them.

I am currently looking at the Constructor API. For instance, the following call is technically fine:

FtpServer("127.0.0.1", my_stream);

However, this only sets the "normal" log stream, not the error stream, creating the following situation:

  • normal messages are directed to my_steam
  • error messages are directed to std::cerr by default

I don't really like that, I think any user who passes only 1 stream would expect both the normal output and error-output would end up in that stream. I suggest the following:

  • Only passing 1 stream should make both outputs be redirected into that stream
    or
  • The API should force the user to also set the error stream, if they set the normal output stream

What do you think?

@ShaharHD
Copy link
Author

ShaharHD commented Jan 3, 2025

I let clang tidy check the code. There were no additional warnings, except from the consts. I fixed them.

Still curious as to why you got that error.
I'm using clang-17 and I see no error with that const

I am currently looking at the Constructor API. For instance, the following call is technically fine:

FtpServer("127.0.0.1", my_stream);

However, this only sets the "normal" log stream, not the error stream, creating the following situation:

  • normal messages are directed to my_steam
  • error messages are directed to std::cerr by default

I don't really like that, I think any user who passes only 1 stream would expect both the normal output and error-output would end up in that stream. I suggest the following:

  • Only passing 1 stream should make both outputs be redirected into that stream
    or
  • The API should force the user to also set the error stream, if they set the normal output stream

What do you think?

Added constructor methods to inforce both or none.

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

Successfully merging this pull request may close these issues.

2 participants