vy commented on PR #3839: URL: https://github.com/apache/logging-log4j2/pull/3839#issuecomment-3209220815
> > 1. It duplicates quite some logic (e.g., in `JsonConfigurationFactory`, extensions and suffixes are semantically identical). > > Totally agree. Let’s put a **default** implementation in `ConfigurationFactory` so most factories don’t have to repeat themselves. Please don't – you will still end up having two methods doing the same thing. > I prefer the dedicated `getSupportedExtensions()` API for a few reasons: > > * `isActive()` and `getSupportedTypes()` are **protected**, not public, so they’re not available to consumers; exposing them (or a raw `getFactories()`) would leak internal details. Agreed. Let's make `isActive()` public. > * Implementations are **inconsistent** today: some factories always report active but conditionally restrict types (e.g., Log4j 1 XML returns `xml` only when a property is set), while others return a constant type set but flip `isActive()` based on optional deps. A single public method lets us **normalize** that behavior and present a stable contract. What you're describing is not a problem, but a behavior and it is all good, AFAICT. It is up to the `CF` what to return from `isActive()`. It makes sense `CF.getFactories()` filtered on `isActive()` yields a different outcome when a user has enabled/disabled Log4j 1 support. As a matter of fact, this is a powerful feature that allows `CF`s to dynamically determine their active status. > * In Log4j 3, `isActive()` isn’t needed anymore: optional Maven **modules** provide support for additional formats, not per-factory optional dependencies, so I would propose to remove `isActive()` from Log4j 3. That is a good idea. > * Conceptually, `ConfigurationFactory` should look like a **singleton service**; how it multiplexes concrete factories is an implementation detail. Exposing `getFactories()` would couple users to that detail, whereas `getSupportedExtensions()` gives them exactly what they need without exposing internals. `CF::getSupportedFileExtensions` _only_ hoists `CF::getSupportedTypes` for a handful of users. OTOH, `CF::instances` would enable several more use cases with less API impact. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
