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

review fix: Filter extends Predicate is now working with projects in JDK7 #1808

Merged
merged 1 commit into from
Jan 4, 2018

Conversation

surli
Copy link
Collaborator

@surli surli commented Jan 3, 2018

This PR intends to fix the bug introduced in #1798 which make projects using JDK7 and custom filters fail. The error comes from the usage of a default method: overriding it in AbstractFilter fixes the bug.

@surli surli changed the title fix: Filter extends Predicate is now working with projects in JDK7 review fix: Filter extends Predicate is now working with projects in JDK7 Jan 3, 2018
@pvojtechovsky
Copy link
Collaborator

I wonder how these projects works? Are they compiling java 8 library (spoon) using java 7 compatible compiler?

Anyway if it helps, I am OK to merge it. This PR is OK for me.

@monperrus monperrus merged commit cef5e07 into INRIA:master Jan 4, 2018
@monperrus
Copy link
Collaborator

really good, thanks!

@surli
Copy link
Collaborator Author

surli commented Jan 4, 2018

Ok actually this was not a proper fix: code that implements the interface Filter might be broken too.
So, either I completely remove the extends Predicate on AbstractFilter, or we consider the change as breaking for JDK7.

Are they compiling java 8 library (spoon) using java 7 compatible compiler?

They are using a JDK 8 but the pom.xml target a Java 7 binary, so I assume the default method are not managed in the produced binary leading to a compilation error.

@pvojtechovsky
Copy link
Collaborator

lets remove Predicate from Filter. It is not so great idea - if it breaks compatibility. We can add it again later after some years ... after everybody migrates

@surli
Copy link
Collaborator Author

surli commented Jan 4, 2018

We can add it again later after some years ... after everybody migrates

Actually I thought that AbstractFilter was more used, so we would be able to implement it in there, but no: AbstractFilter is almost a specific kind of Filter, not really an abstract filter.

I think a good first step, might be to refactor the hierarchy of filters to create a real abstract filter class that - in the future - we can enrich if needed.

@pvojtechovsky
Copy link
Collaborator

to refactor the hierarchy of filters to create a real abstract filter class

I think on many places in spoon there is already used Filter implementation using Lambda

anElement.filterChildren((CtAnnotation m) -> val.equals(m.getValue(...)))...

There is no way how to replace it by AbstractFilter, without making code ugly again.

@surli
Copy link
Collaborator Author

surli commented Jan 5, 2018

There is no way how to replace it by AbstractFilter, without making code ugly again.

Hmmm indeed.

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

Successfully merging this pull request may close these issues.

3 participants