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

Simple Logger #30

Merged
merged 10 commits into from
Jan 18, 2025
Merged

Simple Logger #30

merged 10 commits into from
Jan 18, 2025

Conversation

Anand5329
Copy link
Contributor

@EdwardPalmer99 Enacted upon your TODO in utility.
Simple Logger class, inspired form Java's Logger.

Can log on five different levels, with three different filters.
Let me know what you think, and if you'd like to change the design.

I can also add tests if needed!

Copy link
Owner

@EdwardPalmer99 EdwardPalmer99 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I've made a couple of comments. Overall looks good and happy to merge after the comments are addressed. Look forward to more PRs in the future! Lots to do. Currently working on implementing pointers. Welcome any thoughts you have.

src/utility/Logger.cpp Show resolved Hide resolved
src/utility/Logger.hpp Outdated Show resolved Hide resolved
src/utility/Logger.hpp Outdated Show resolved Hide resolved
src/utility/Logger.hpp Outdated Show resolved Hide resolved
src/utility/Logger.cpp Outdated Show resolved Hide resolved
src/utility/Logger.cpp Outdated Show resolved Hide resolved
src/utility/Logger.cpp Outdated Show resolved Hide resolved
src/utility/Logger.hpp Outdated Show resolved Hide resolved
src/utility/Logger.hpp Show resolved Hide resolved
@EdwardPalmer99 EdwardPalmer99 added the enhancement New feature or request label Jan 9, 2025
Copy link
Owner

@EdwardPalmer99 EdwardPalmer99 left a comment

Choose a reason for hiding this comment

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

Thanks for adding some additional commits. I will approve and merge this PR. Overall, it's very good and we really need this in Eucleia since there is an awful lot going on. I'm going to make another PR to address some of my new minor comments and to add the log level to our CLIParser so the user can specify the logging level

{
public:
enum class Filter
{
Copy link
Owner

Choose a reason for hiding this comment

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

I think the Filter is overkill. Personally, I would just use a second Level variable. Then you can compare the level of the message being passed in to the level of our Level variable since enum values are just increasing integers.


void log(Level level, const char *file, unsigned int line, const char *func, std::string_view message);

inline void error(const char *file, unsigned int line, const char *func, std::string_view message) { log(Level::error, file, line, func, message); };
Copy link
Owner

Choose a reason for hiding this comment

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

It looks a little messy doesn't it? I think specifying the file, line, func was a mistake on my part. Also probably overkill on the log options: [debug, info, warning, error] are enough I think

* @author Anand Doshi
* @date 2025-01-08
*
* @copyright Copyright (c) 2025
Copy link
Owner

Choose a reason for hiding this comment

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

I've done some more thinking. I think the logger should be a singleton. We need an instance() public method and the constructor should be protected to prevent initialisation by the user. Then the info, error, ... methods become static methods calling instance().log(...)

@EdwardPalmer99 EdwardPalmer99 merged commit bbc597e into EdwardPalmer99:master Jan 18, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants