-
Notifications
You must be signed in to change notification settings - Fork 815
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
Accidental breaking API change #892
Comments
I guess, it will not solve the issue that is caused by method overloading resolution with the nastiest (IMHO) "innovation" ever introduced in Java ecosystem: lambda expression.. Compiler cannot select the right method in this case because null is null and there's no way to distinguish a null Predicate from a null PrometheusScrapeRequest
in the same way this statement works as expected
Other solutions require different method names e.g., |
Having two methods seems like the wrong approach. What about...
Then you can implement...
I believe this would also work with a lambda. |
Why 2 methods are a wrong approach ? My comment in the code
|
@ganzuoni you're correct, the code is broken (that's what I get for trying to do it on the fly/from my phone.) |
Signed-off-by: Fabian Stäber <fabian@fstab.de>
I merged #898 and restored the The most common case, The multi target pattern is not a common use case, while filtering by metric name is common. So it's reasonable to have a simple lambda expression |
With #873 we accidentally introduced a breaking API change. Before the change, you could pass a metric name filter as a lambda expression:
Now, this fails:
Same for
(Multi)Collector.collect(name -> name.equals("..."))
.@ganzuoni what do you think is a good way to fix this? I know your initial PR had a method
getRequestPath()
, which we removed because it wasn't needed. I guess we could add it again to thePrometheusScrapeRequest
interface? What do you think?The text was updated successfully, but these errors were encountered: