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

Allows replacement of any reference with another one. #391

Merged
merged 6 commits into from
Dec 2, 2015
Merged

Allows replacement of any reference with another one. #391

merged 6 commits into from
Dec 2, 2015

Conversation

GerardPaligot
Copy link
Contributor

Note: This PR introduces CtObject interface in the second commit and breaks compatibility with the usage of getParent() method.

Closes #240

@monperrus
Copy link
Collaborator

Thanks for this PR.

I disagree with the introduction of CtObject:

  • the difference of responsibility between CtElement and CtObject is unclear, even in the names.
  • the necessary changes in the tests with the casts to CtElement are not nice

What about a simple CtReplaceable interface for both elements and references?

@GerardPaligot
Copy link
Contributor Author

the difference of responsibility between CtElement and CtObject is unclear, even in the names.

I can replace it by CtReplaceable.

the necessary changes in the tests with the casts to CtElement are not nice

I agree but necessary. Now, it is possible to have a CtElement or a CtReference as parent.

What about a simple CtReplaceable interface for both elements and references?

It's already the case but named CtObject. I can change it.

@GerardPaligot
Copy link
Contributor Author

If continuous integrations are ok, I'm okay too.

But if we merge this PR, I vote to make a major version at the next release.

@GerardPaligot
Copy link
Contributor Author

The continuous integration server non regression at large scale is okay with this PR too.

capture d ecran 2015-11-19 a 11 25 30

}

for (CtTypeReference<?> reference : references) {
reference.replace(factory.Type().createReference(reference.getQualifiedName()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace only one reference.

@GerardPaligot
Copy link
Contributor Author

TODO: Change to 5.0.0-SNAPSHOT in pom.xml

@GerardPaligot
Copy link
Contributor Author

TODO: Adds some test for parent of a CtReference.

final CtParameterReference<?> aParameterReference = (CtParameterReference<?>) variableRead.getVariable();
final CtFieldReference<?> aFieldReference = aTacos.getField("field").getReference();

assertEquals(variableRead.getVariable(), aParameterReference);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flip expected, actual

@monperrus
Copy link
Collaborator

I agree with the jump to 5.0.0-SNAPSHOT, even if the tests show that nothing specified is broken, it may impact client processors of CtElement

@GerardPaligot
Copy link
Contributor Author

TODO: Updates documentation

@GerardPaligot
Copy link
Contributor Author

@monperrus PR ok.

@GerardPaligot
Copy link
Contributor Author

The continuous integration server non regression at large scale is always okay with this PR.

capture d ecran 2015-12-02 a 10 08 06

monperrus added a commit that referenced this pull request Dec 2, 2015
@monperrus monperrus merged commit 128dd33 into INRIA:master Dec 2, 2015
@GerardPaligot GerardPaligot deleted the fix_240 branch December 2, 2015 10:35
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