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.

Sign up for GitHub

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

Jump to bottom

[SofaGraphComponent] Refactor the SceneChecker and add a new SceneChecker to test dumplicated names. #392

Merged
merged 6 commits into from
Sep 15, 2017
Merged

[SofaGraphComponent] Refactor the SceneChecker and add a new SceneChecker to test dumplicated names. #392

merged 6 commits into from
Sep 15, 2017

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Sep 7, 2017 •
edited by guparan
Loading

Hi all,

This PR is implementing what was discussed in in #362

CHANGELOG:

  • refactor the SceneChecker object for more modularity. Adding new checks should be easier.
  • add a SceneCheckDuplicateName that warns user if the scene contains duplicated names.
  • add the corresponding tests.
  • use the new version in runSofa.

This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@damienmarchal
[SofaGraphComponent] Add a base class for SceneCheck & refactor Scene…
23878e3 
…Checkr.

We have more and more check to implement. Instead of hardcoding
them into the SceneCheckerVisitor we now have a base class to
use.
@damienmarchal
[SofaGraphComponent] Propagate the #include changes introduced in (CONT)
63e0b5c
@damienmarchal
[SofaGraphComponent] Re-implement the SceneChecker old behavior (foll…
d285eb2 
…ow-up SceneCheckerRef)

# Conflicts:
#	modules/SofaGraphComponent/SceneCheckerVisitor.cpp
@damienmarchal
[all] Update the SceneChecker to use shared_ptr & add duplicate name …
b281f15 
…checking

# Conflicts:
#	SofaKernel/modules/SofaSimulationGraph/DAGNode.cpp
#	modules/SofaGraphComponent/SceneCheckerVisitor.cpp
@damienmarchal
[SofaGraphComponent] Update the tests & runSofa to use the new SceneC…
a473d5a 
…hecker.
@damienmarchal damienmarchal added enhancement About a possible enhancement project: UX pr: status to review To notify reviewers to review this pull-request labels Sep 7, 2017
@damienmarchal damienmarchal changed the title Refactor the SceneChecker and add a new SceneChecker to test dumplicated names. [SofaGraphComponent] Refactor the SceneChecker and add a new SceneChecker to test dumplicated names. Sep 7, 2017
@damienmarchal
Copy link
Contributor Author

Hi all,
No regression on this one...so I ask for fast-merge :)

@damienmarchal damienmarchal added the pr: fast merge Minor change that can be merged without waiting for the 7 review days label Sep 8, 2017
@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Sep 13, 2017
@hugtalbot
Copy link
Contributor

[ci-build]

1 similar comment
@damienmarchal
Copy link
Contributor Author

[ci-build]

@damienmarchal damienmarchal merged commit 38412b9 into sofa-framework:master Sep 15, 2017
@damienmarchal
Copy link
Contributor Author

This one seems to be ok & taggued ready. So I merge it.

guparan
guparan reviewed Sep 18, 2017
View reviewed changes
modules/SofaGraphComponent/SofaGraphComponent_test/SceneChecker_test.cpp
checker.validate(root.get()) ;
}
}

void checkDuplicatedNames()
{
EXPECT_MSG_EMIT(Error) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

@damienmarchal, I think you meant EXPECT_MSG_NOEMIT here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct #405

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha didn't see that yet, will do ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was so ashamed to have merged something with obviously broken test that I quickly fix it ;)

@damienmarchal damienmarchal deleted the addSceneCheckerDuplicate branch September 20, 2017 18:33
@guparan guparan added this to the v17.12 milestone Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan left review comments

Assignees
No one assigned
Labels
enhancement About a possible enhancement pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.
3 participants
@damienmarchal @hugtalbot @guparan