Skip to content
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

[FEATURE] ToString's onlyExplicitlyIncluded configurable from lombok.config #2849

Closed
mickroll opened this issue May 20, 2021 · 7 comments
Closed
Assignees
Labels
accepted The issue/enhancement is valid, sensible, and explained in sufficient detail critical enhancement
Milestone

Comments

@mickroll
Copy link

mickroll commented May 20, 2021

As there is the @ToString(onlyExplicitlyIncluded = true/false) option already, it would make sense to have the ability of setting it once across the project at the lombok.config, eg. via lombok.toString.onlyExplicitlyIncluded = true/false

Justification:

  • Improved signal-to-noise ratio
  • Better readability (arguably)
  • Helps keeping project convention consistent
  • would work similar to and alongside with lombok.toString.doNotUseGetters, lombok.toString.includeFieldNames and lombok.toString.callSuper

BTW. Thanks for adding this option. It really helps preventing data leakage into log files 👍

@rzwitserloot
Copy link
Collaborator

Fair enough. Barely got across the value vs. maintenance burden hurdle, but made it.

@rzwitserloot rzwitserloot self-assigned this Dec 21, 2021
@rzwitserloot rzwitserloot added accepted The issue/enhancement is valid, sensible, and explained in sufficient detail enhancement labels Dec 21, 2021
@rzwitserloot rzwitserloot added this to the next-version milestone Dec 21, 2021
@mickroll
Copy link
Author

@rzwitserloot Great 👍

@janrieke
Copy link
Contributor

I just noticed that commit 6d2a474 that implemented this change breaks a lot of the tests. Didn't investigate further on the cause.

janrieke added a commit to janrieke/lombok that referenced this issue Jan 3, 2022
@mickroll
Copy link
Author

So... maybe reopen this feature?

@rspilker rspilker reopened this Jan 11, 2022
@rspilker
Copy link
Collaborator

@janrieke Any ideas on why this breaks so many tests? I haven't verified that statement myself.

@rspilker
Copy link
Collaborator

rspilker commented Jan 14, 2022

Yes, the reason is that this feature exposed a three year old bug, where we cast the default parameter to the wrong type, and the catch block then returns false for the includeFieldNames, which is one of the rare cases where the default of a boolean flag is true instead of false.

The proper solution is to fix the offending code, which we're doing right now, and to change the after-files for this specific feature, that should have included the field names.

So apart from the after-files, the pull request itself is great.

@rzwitserloot
Copy link
Collaborator

Well, I checked this out, and I conclude that those tests do fail, and must have always failed. Except they didn't. :raise eyebrow:.

I, uh, fixed the impossible thingie, see commits 2685d37 and daee44d. I'm getting all greens across the board now, without reverting the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The issue/enhancement is valid, sensible, and explained in sufficient detail critical enhancement
Projects
None yet
Development

No branches or pull requests

4 participants