sosy-lab / java-common-lib

SoSy-Lab Java Common Library
https://www.sosy-lab.org
Apache License 2.0
12 stars 11 forks source link

Change LogLevelFilter.java constructor modifier to public #13

Closed BCSol closed 6 years ago

BCSol commented 7 years ago

I am quite sure that the modifier should be public, in order to allow users to reuse this filter.

PhilippWendler commented 7 years ago

The general policy is to make things public only if necessary (cf. Effective Java, Item 13).

Is there a particular reason this should become public? Can this purpose maybe be served better in another way?

BCSol commented 7 years ago

For the VCloud I had to implement a new Handler in order to support logging to a compressed file. And I wanted to reuse the LogLevelFilter as blacklist for a specific handler configuration. Sadly this was not possible.

I can't think of another 'better way', Maybe some kind of builder method static Filter getBlackListFilter(List<Level> exclude), but that would be overcomplicated.

PhilippWendler commented 7 years ago

A static factory method in a common utility class would typically be nice solution and not overcomplicated at all, check for example Guava's Predicates class.

But first we should discuss the underlying problem: Do you want to create a BasicLogManager by hand (without a Configuration instance), or do you need to do this only because there is a missing feature (which could probably be added)?

BCSol commented 7 years ago

Regarding the underlying problem: The VCloud does use its own Configuration implementation (I will file that issue). So I can't simply pass in the Configuration. Additionally BasicLogManager does not support compressed file logging.

Since the java logger protected access is deprecated, a builder using ´BasicLogManager(Logger logger)´ was the way to go. I was able to reuse the ConsolLogFormatter and FileLogFormatter, but not the Filter.

PhilippWendler commented 7 years ago

The VCloud does use its own Configuration implementation (I will file that issue). So I can't simply pass in the Configuration.

In fact, for such situations the use of LoggingOptions was intended, but I notice that its constructor also requires a instance Configuration, which should better have been in a factory method. But the fix is easy: just add a protected parameter-less constructor to LoggingOptions, then the VerifierCloud can inherit from this class and specify its values as it wishes.

Additionally BasicLogManager does not support compressed file logging.

This is a feature that could simply be added.

Since the java logger protected access is deprecated, a builder using ´BasicLogManager(Logger logger)´ was the way to go.

This is possible, but if your end goal is use as much from this package as possible anyway, why should we change something now that is only an intermediate solution for you (but has drawbacks for our API), instead of aiming for the final solution?

PhilippWendler commented 6 years ago

Should this be closed?

BCSol commented 6 years ago

Yes this should be closed.