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

Added exceptionLevel to allow different log level for exceptions #364

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

Conversation

fnmps
Copy link

@fnmps fnmps commented Sep 18, 2023

Solution for issues: #259 #236
Solution allows for new property exceptionLevel on Loggable annotation, which, if defined, will log the exception on the given log level, else it will use the log level defined on the value property, making the solution backward compatible.

* Log level for exceptions. If not defined then the log level of value is used.
* @return The log level
*/
int exceptionLevel() default -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be a constant here, probably Loggable.WARNING

Copy link
Author

@fnmps fnmps Sep 19, 2023

Choose a reason for hiding this comment

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

@andreoss Creating a constant on this class would cause it to be visible to developers using the Loggable annotation, which could cause confusion. At most I could create a class for the constant, so it would be less confusing to developers using the library.
Should I proceed with that implementation or is it overkill?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fnmps I don't quite get why we need a class. -1 means default, so you can add Loggable.DEFAULT. onException probably would be a better name than exceptionLevel

src/main/java/com/jcabi/aspects/aj/MethodLogger.java Outdated Show resolved Hide resolved
@fnmps fnmps requested a review from andreoss September 19, 2023 21:25
@fnmps fnmps marked this pull request as draft September 29, 2023 21:11
@fnmps fnmps marked this pull request as ready for review September 29, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants