"excludeFields" is applied to all fields and not just to the fields in "Library"
SageDroid opened this issue · 7 comments
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
- [11.2.2] Used library version
Checklist
- Searched for similar issues
- Checked out the sample application
- Read the README
- Checked out the CHANGELOG
- Read the MIGRATION GUIDE
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. 🤔
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?
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. 👍
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.
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
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?
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