mikepenz / AboutLibraries

AboutLibraries automatically collects all dependencies and licenses of any gradle project (Kotlin MultiPlatform), and provides easy to integrate UI components for Android and Compose-jb environments
http://mikepenz.github.io/AboutLibraries/
Apache License 2.0
3.66k stars 423 forks source link

"excludeFields" is applied to all fields and not just to the fields in "Library" #1014

Closed SageDroid closed 2 months ago

SageDroid commented 2 months ago

About this issue

The documentation describes excludeFields as follows:

Defines fields which will be excluded during the serialisation of the metadata output file. Any field as included in the [com.mikepenz.aboutlibraries.plugin.mapping.Library] can theoretically be excluded.

Problem: excludeFields is applied to all fields and not just to the fields in Library.

Example: If you exclude name (for Library.name), the License.name, Developer.name and Organization.name properties are also removed from the output. This means that it is no longer possible to parse the libraries and licenses at runtime, as License.name and Organization.name are not optional.

Details

Checklist

mikepenz commented 2 months ago

Thank you for the ticket.

I believe we should make sure not to break the current behavior for users. It probably would be better if we add new configuration options for the adjusted behavior.

Based on the ticket information it appears that the goal would actually be to be more specific on what to exclude. E.g. be able to exclude the name of an organisation or similar.

The current proposed PR would solely limit any exclusions on the Library, removing the possibility to remove fields from child elements. 🤔

SageDroid commented 2 months ago

Thanks for your reply (and this brilliant library)!

I generally agree with you. The changes in the pull request were based on my understanding of the documentation. I did not assume that the current behavior was intended.

Further configuration options would make using the library more complex. Especially if the old behavior and the new one exist in parallel.

One idea that just popped into my head is that we could allow to qualify the field name in excludeFields by the class name (e.g. Library.name). Unqualified field names would continue to be applied globally. This would offer full configurability and nobody would have to change their existing configuration.

What do you think?

mikepenz commented 2 months ago

The later suggestion, using fully qualified field names is a great and would in my opinion tick all boxes. it won't break existing implementations, but also provide full flexibility for other usecases. 👍

SageDroid commented 2 months ago

I have updated my PR. The current behavior is still supported. In addition, it is now possible to specify fully qualified field names. In my example, that would be:

excludeFields = ["com.mikepenz.aboutlibraries.plugin.mapping.Library.name"]

I can also update the documentation (and possibly the readme - or do you do that with the release?) if you are okay with the new implementation.

mikepenz commented 2 months ago

If possible can we make it so it's not requiring the package but ["Library.name"]? would be enough? And yes please, feel free to also improve the docs

SageDroid commented 2 months ago

Sorry I understood your previous reply to mean that you prefer fully qualified names.

I have updated the PR. The added list of allowed qualifiers (class names), ensures that it is still possible to exclude field names with dots (may occur in the licenses). All class names that are currently used in ResultContainer are permitted. I have refrained from building the list with reflection in order to keep the code easy to understand.

It's now excludeFields = ["Library.name"]

Is this solution ok for you?

mikepenz commented 2 months ago

Thanks. I think that should now be fine. Haven't reviewed the code in detail, but I hope I'll find some time later this week to do so