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 set_logger_level function #80

Merged
merged 8 commits into from
Jul 13, 2024
Merged

Add set_logger_level function #80

merged 8 commits into from
Jul 13, 2024

Conversation

renatillas
Copy link
Contributor

As I've noticed the logging library has been updated with the new set_logger_level function. I've thought that adding a wrapper around that function in wisp would be nice, altough it could be divided into many smaller functions as set_log_level_to_info() and the like. What do you think?

Copy link
Collaborator

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Nice idea, thank you.

src/wisp.gleam Outdated
/// [1]: https://www.erlang.org/doc/man/logger
///
pub type LogLevel =
logging.LogLevel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you create a new custom type here for the log level? To avoid making that library part of the public API, and to avoid the users having to import 2 modules.

src/wisp.gleam Outdated
///
/// [1]: https://www.erlang.org/doc/man/logger
///
pub fn set_logger_level(log_level: LogLevel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return annotation is missing!

@renatillas
Copy link
Contributor Author

Comments solved!

@renatillas renatillas requested a review from lpil June 21, 2024 15:07
Copy link
Collaborator

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Would you mind updating the changelog also

@renatillas
Copy link
Contributor Author

Thank you! Would you mind updating the changelog also

Done!

@renatillas renatillas requested a review from lpil June 24, 2024 10:23
Copy link
Collaborator

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

CHANGELOG.md Outdated Show resolved Hide resolved
src/wisp.gleam Outdated
Emergency
Alert
Critical
Error
Copy link
Collaborator

@lpil lpil Jun 30, 2024

Choose a reason for hiding this comment

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

Ah! This is overriding the prelude Error type resulting in type errors. Perhaps they can all be suffixed with Level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uuhh my lsp didn't caught that, my bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@renatillas renatillas requested a review from lpil June 30, 2024 18:40
Copy link
Collaborator

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Lovely!!! Thank you!

@lpil lpil merged commit 4d346fd into gleam-wisp:main Jul 13, 2024
1 check failed
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