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

Allow to skip certain signatures from bundled signature files #252

Open
kwin opened this issue Jan 4, 2025 · 2 comments · May be fixed by #253
Open

Allow to skip certain signatures from bundled signature files #252

kwin opened this issue Jan 4, 2025 · 2 comments · May be fixed by #253

Comments

@kwin
Copy link
Contributor

kwin commented Jan 4, 2025

Sometimes not all signatures from a bundled signature files are relevant.
For example jdk-unsafe contains also String.format(...) methods. However in most of the cases the Locale is not relevant (i.e. when only using String format specifier %s).
It would be nice to allow to either completely skip checking individual signatures or at least lower the severity of the log message (from error to warn).

I know about @de.thetaphi.forbiddenapis.SuppressForbidden but this would need to be used everywhere in the code where the affected signature is being used.

This is related to #219 but requires a more granular parametrisation.

kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 4, 2025
@kwin kwin linked a pull request Jan 4, 2025 that will close this issue
kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 4, 2025
kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 4, 2025
kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 4, 2025
kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 4, 2025
@dweiss
Copy link
Contributor

dweiss commented Jan 5, 2025

However in most of the cases the Locale is not relevant (i.e. when only using String format specifier %s).

This is only true if you're converting objects whose toString methods are locale-insensitive, which may not be true in all cases. If you allow tinkering with selective options, you'll soon run into arguments about what to include or exclude, which is a waste of time. I'd say just take it all or add warning suppression annotation where you think your code is valid (or a method that redelegates).

Uwe may disagree but my opinion to this is pretty much consistent with google's java formatter - fewer configurable options lead to less bikeshedding over what to enable/disable, etc.

@kwin
Copy link
Contributor Author

kwin commented Jan 5, 2025

There are false positives and different approaches how to deal with those. I agree that going through each use case manually and individually mark as safe with the suppress annotation or just use the locale/timezone specific method is the safest approach.
However for large codebases this is often way too much effort. So you might be interested in first checking certain signatures which you know for sure cause issues. The other you can at least temporarily ignore with this option. I don't think that this adds too much complexity as this is opt-in, i.e. nothing changes when you don't configure it explicitly.

Regarding

This is only true if you're converting objects whose toString methods are locale-insensitive, which may not be true in all cases.

I would say that this needs to be fixed in the toString() method instead because even when using the locale specific String.format this is not passed to the called toString() method (as this never takes any arguments).

kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 6, 2025
kwin added a commit to kwin/forbidden-apis that referenced this issue Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants