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

Wildcard method signatures #188

Merged
merged 6 commits into from
Dec 16, 2021
Merged

Wildcard method signatures #188

merged 6 commits into from
Dec 16, 2021

Conversation

madrob
Copy link
Contributor

@madrob madrob commented Dec 13, 2021

Allow specifying all methods regardless of argument types, for example:

List.of(**) would match all of the factory methods. This is useful when you want to match those methods without listing them individually and also not forbidding the entire class.

I did not see unit tests for this class, so I wasn't sure where to add testing.

@uschindler
Copy link
Member

Hi Mike,

thanks for this PR! Looks like a good thing. I have the feeling that there was already an issue about this! What I really like is that it will trigger a method not found issue if you add a false signature like some.ClazzThatExists#methodWithTypo(**) (not matching any method).

I'd like to make some antunit test for this before merging it in -- to be sure That should be easy. I am a bit busy today with log4j, but will check out for sure.

Uwe

@uschindler
Copy link
Member

Of course an update for the documentation is required.

@uschindler uschindler self-assigned this Dec 13, 2021
@uschindler uschindler added this to the 3.3 milestone Dec 13, 2021
@uschindler
Copy link
Member

Hi @madrob,
I rewrote the parser a bit to get rid of the "ignoreMethodArgs" boolean. It is also lenient to whitespace like ASM's parser.

I also added a few tests.

Next I will update documentation, but code-wise it's ready.

@uschindler
Copy link
Member

It's now ready. Just give me your go, @madrob. Does this fit your needs?

@madrob
Copy link
Contributor Author

madrob commented Dec 16, 2021

That looks pretty cool from initial code review. I think it should be good to merge, but I won’t get a chance to test this until next week.

@uschindler
Copy link
Member

OK, will merge. Are you planning to use this in Solr/Lucene or for own projects?

I am not yet sure if those 2 changes I merged today should be a new release, or if I should wait for Java 18.

@uschindler uschindler merged commit 7f7767c into policeman-tools:main Dec 16, 2021
@madrob
Copy link
Contributor Author

madrob commented Dec 17, 2021

My current plan was to use it for Solr.

@uschindler
Copy link
Member

Give me a ping if you need it urgently, then I will do a release.

As usual before release there need to be some checks:

  • Check for new commons-io versions and analyze them
  • Check ASM version bugfixes

@madrob
Copy link
Contributor Author

madrob commented Dec 17, 2021

I’m on vacation until January, so no rush at all.

would you be interested in adding some commons io signatures to the default list? Specifically, I’m looking at IOUtils.copy since they can be replaced by jdk native methods

@uschindler
Copy link
Member

would you be interested in adding some commons io signatures to the default list? Specifically, I’m looking at IOUtils.copy since they can be replaced by jdk native methods

I would only add real unsafe commons-io methods. The copy methods are fine, just unneeded with newer JDKs. We may ship in future with other "recommended replacements" signatures. Like forbid java.io.File (see Lucene) and refer to java.nio.file.Path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants