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
Support for CustomLog #2086
Support for CustomLog #2086
Conversation
Hi all, this is a great addition and that we could use on our project. When could someone take a look at this? |
Apologies for the late reply. This is heading in the right direction: IdentifierNameYour addition to the config system with java identifiers ( However... my preferred suggestion for how to tackle this means you can't use IdentifierName for it.
|
NB: One downside to the above proposal is that use of custom logger which does not allow topics would result in a runtime-only exception (in that the glue method would throw if topic was anything but empty), whereas if it's all configurable, lombok could generate an error sooner, but this seems like a fair price to pay. Having more than one config key does not play well with the cascading nature of the config system. Imagine you set 'fail if topic is set' in your project root, and then in some subdir of your project root you write a lombok config that sets the logger factory method to use (to a different logger that DOES support topics), but fails to reset the 'fail if topic is set' flag. By having only 1 configkey, this problem is avoided. |
IdentifierNameI will be happy to apply it to all places and will create a separate PR for it. There are only two other places anyway (marked TODO by my PR).
|
This looks great to me. If you treat the configkey as a string and parse it out in the customlogger handler with a regexp, I don't think we'd run into any performance issues there. |
I tested the latest from ataccama:master and it worked fine for me. The one downside is that the IntelliJ plugin will need to be updated since it didn't recognize my custom logger. |
@rzwitserloot, I would also like to add support to the IDEA plugin. Have you had time to review my implementation? Do you expect the current proposal to change considerably? |
Gonna look at this now. |
Just a random question @juriad : I noticed you indented the empty lines. Whilst git recommends against this, the way both Roel and I use editors, we like doing this, and it is indeed the lombok 'house style'. This might be the first time I noticed a PR that got this right. Did you notice we do it this way and you copied the style, or, is it also how you write code? |
I've reviewed your code, made some very minor fixes, and updated the docs + changelog. However, I noticed you haven't added your name to the AUTHORS file yet. For legal purposes we do require it (as part of doing that, you're saying that you're okay with your code being integrated into lombok, as per the lombok open source license, and that you wrote it / have full and permanent permission to relinquish the copyright like that). @juriad can you review my docs, and add your name to the AUTHORS file? |
@juriad any progress on this? |
I started to implement support for @CustomLog annotation as discussed in #843.
This PR proposes the new annotation and how it can be configured. This is work in progress; I haven't tested it at all yet. I'd like to discuss the design, my approach, and the code style.