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

Add missing overrides for ModifierVisitorAdapter #301

Merged
merged 1 commit into from
May 4, 2016

Conversation

thejohnfreeman
Copy link
Contributor

Closes #300

@ftomassetti
Copy link
Member

It seems to not compile:

[ERROR] /home/travis/build/javaparser/javaparser/javaparser-core/src/main/java/com/github/javaparser/ast/visitor/ModifierVisitorAdapter.java:[804,12] cannot find symbol
  symbol:   class R
  location: class com.github.javaparser.ast.visitor.ModifierVisitorAdapter<A>

@ftomassetti
Copy link
Member

I think it makes sense but we should fix the return type of the second method

@thejohnfreeman
Copy link
Contributor Author

thejohnfreeman commented May 3, 2016

Yeah, sorry about that. I fixed that and tried to add a test. Let me know if it doesn't meet the standards or is in the wrong place. This is my first time using JBehave.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 61.172% when pulling fcf024f on thejohnfreeman:master into 9b239aa on javaparser:master.

@ftomassetti
Copy link
Member

No problem, tests are there to help us catch these things :)


public class ModifierVisitorAdapterTest {

@Then("ModifierVisitorAdapter requires no overrides")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what this test is doing. It instantiate an object and then it verifies it is not null?

@thejohnfreeman
Copy link
Contributor Author

Yes. The real test is that the construction compiles. If ModifierVisitorAdapter is missing any overrides like it was before this patch, then the construction will not compile. I added the assertion just so that the test has at least one. I'll make this test look however you want, just tell me.

@ftomassetti
Copy link
Member

I would suggest we just remove the test and merge the change as it is.

Thanks for your help on this one!

@thejohnfreeman
Copy link
Contributor Author

As you wish.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 61.172% when pulling 52dc8de on thejohnfreeman:master into 9b239aa on javaparser:master.

@ftomassetti
Copy link
Member

Looks perfect. Thanks again!

@ftomassetti ftomassetti merged commit 490f41c into javaparser:master May 4, 2016
@matozoid matozoid added this to the next release milestone Jun 1, 2016
ftomassetti added a commit to ftomassetti/javaparser that referenced this pull request Jan 10, 2018
…of_javassist_field

fixed solving generic type of JavassistFieldDeclaration
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.

None yet

4 participants