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: better tests for CtScanner #1642

Merged
merged 3 commits into from
Oct 31, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

@pvojtechovsky pvojtechovsky commented Oct 23, 2017

This PR contains fixes of CtScanner, CtScanner test and Spoon model annotations related to CtScanner.

@pvojtechovsky pvojtechovsky changed the title review2: fix: CtScanner and it's tests WiP: fix: CtScanner and it's tests Oct 24, 2017
@pvojtechovsky
Copy link
Collaborator Author

@tdurieux @monperrus How to fix missing calls of scan(getComments()) in CtScanner? I guess it is correct to add them everywhere.
There is just question where?
A) as first scanned attribute
B) as last scanned attribute
C) on different position

Actually all 3 cases are there in CtScanner. Is it intentional? Should we unify it?

@monperrus
Copy link
Collaborator

I would say A).

@pvojtechovsky pvojtechovsky force-pushed the fixCtScanner branch 2 times, most recently from 3e6a69f to a4a4abd Compare October 24, 2017 19:32

@Override
@UnsettableProperty
<T extends CtActualTypeContainer> T setActualTypeArguments(List<? extends CtTypeReference<?>> actualTypeArguments);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This property seems to be settable, because when I made it unset-able by

class CtIntersectionTypeReferenceImpl ... { ...
	public <C extends CtActualTypeContainer> C setActualTypeArguments(List<? extends CtTypeReference<?>> actualTypeArguments) {
		return (C) this;
	}

Then TypeReferenceTest#testRecursiveTypeReference started to fail.
But I am not able to decide yet what is correct. Test or Unsettable property. I do not understand spoon/java model around CtIntersectionTypeReference.

So check this one carefully.

@pvojtechovsky
Copy link
Collaborator Author

I suggest to add calls of scan(getComments()) in next PR. This one is big enough.

@pvojtechovsky pvojtechovsky changed the title WiP: fix: CtScanner and it's tests review2: fix: CtScanner and it's tests Oct 24, 2017
@monperrus
Copy link
Collaborator

In general I'm fine with the idea of this PR.

However, it's really big for good code review.

Since it contains the changes of #1627 do we concentrate on #1627 first? or can we remove the moetamodel from this one and merge this one independently?

@pvojtechovsky
Copy link
Collaborator Author

#1627 Meta model cannot be removed, because the test of new CtScanner is based on this new metamodel.

I agree to focus on meta model first. And I am open to spent on that discussion some days, because I found metamodel of spoon as quite interesting feature for future maintenance and generation of spoon code - so it should be well designed, with respect to your meta model knowledge of spoon too!

@pvojtechovsky pvojtechovsky changed the title review2: fix: CtScanner and it's tests WiP: fix: CtScanner and it's tests Oct 28, 2017
@pvojtechovsky pvojtechovsky force-pushed the fixCtScanner branch 2 times, most recently from 40a1ce2 to d025466 Compare October 29, 2017 17:25
@INRIA INRIA deleted a comment from spoon-bot Oct 29, 2017
@INRIA INRIA deleted a comment from spoon-bot Oct 29, 2017
@pvojtechovsky pvojtechovsky changed the title WiP: fix: CtScanner and it's tests review: fix: CtScanner and it's tests Oct 29, 2017
@pvojtechovsky
Copy link
Collaborator Author

So now metamodel is merged and this one is next in the review queue ;-)

@pvojtechovsky pvojtechovsky changed the title review: fix: CtScanner and it's tests review2: fix: CtScanner and it's tests Oct 29, 2017
@pvojtechovsky
Copy link
Collaborator Author

I extracted PR #1668 to simplify this one. So lets merge #1668 first

@pvojtechovsky pvojtechovsky changed the title review2: fix: CtScanner and it's tests review: fix: CtScanner and it's tests Oct 30, 2017
@pvojtechovsky
Copy link
Collaborator Author

Now this PR contains only fix of CtScannerTest and fixes of spoon model, which were found by that fixed test.

@monperrus monperrus changed the title review: fix: CtScanner and it's tests review: fix: better tests for CtScanner Oct 30, 2017
@monperrus
Copy link
Collaborator

just reviewed the test and added some documentation.

really good work, thanks!

I'm OK to merge, are you done?

@pvojtechovsky
Copy link
Collaborator Author

yes I am done. Please merge it soon. I will then rebase other PRs so I can clean my workspace and continue with next features ;-)

@monperrus monperrus merged commit 7fc402f into INRIA:master Oct 31, 2017
@pvojtechovsky pvojtechovsky deleted the fixCtScanner branch September 1, 2018 07:24
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.

2 participants