Skip to content

[SceneChecking] MissingRequiredPlugins: Adapts messages for xml and python users#3033

Closed
EulalieCoevoet wants to merge 2 commits intosofa-framework:masterfrom
EulalieCoevoet:pr-missingRequiredPlugins
Closed

[SceneChecking] MissingRequiredPlugins: Adapts messages for xml and python users#3033
EulalieCoevoet wants to merge 2 commits intosofa-framework:masterfrom
EulalieCoevoet:pr-missingRequiredPlugins

Conversation

@EulalieCoevoet
Copy link
Copy Markdown
Contributor

This PR improves the message about missing required plugins for python users. Before it was printing lines to cut and paste in the scene (which is really nice!), but only for xml users. Now I propose to check the scene's extension and adapt the message.

Here's an example for python:

image


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

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

@damienmarchal
Copy link
Copy Markdown
Contributor

Thanks @EulalieCoevoet for this nice contribution.

It is still unclear to me if instead of having the extension passed by changing the SceneChecker API it would not be a better to use Base::getInstanciationSourceFileName().

@alxbilger, what is your opinion ?

[ci-build][with-scene-tests]

@EulalieCoevoet
Copy link
Copy Markdown
Contributor Author

Sure, if such method exists I should use that. Thanks for pointing it out.

@damienmarchal
Copy link
Copy Markdown
Contributor

damienmarchal commented Jun 14, 2022

Yes the method exists... but, contrary to the XML loader, the SofaPython3 scene loader does not use the setInstanciationSourceFileName to set the filename associated with the root node.
This can be done in a simple PR at SofaPython3.

EDIT: I made a quick pr to for SofapythoN3: sofa-framework/SofaPython3#271

@alxbilger
Copy link
Copy Markdown
Contributor

This is how I would do it:

  1. Separate the SceneCheckMissingRequiredPlugin class into 3: 1) an abstract base class, 2) a derived class printing the XML summary, 3) a derived class printing the Python summary.
  2. Move the XML class near the XML scene loader
  3. Move the Python class in SofaPython3
  4. Add (optionally) a new instance of the classes in each scene loader (see void addListener( Listener* l ))

I admit this is quite complex to do. The reason is that all the scene checkers are all grouped in the same module, and SceneCheck is not in Core (but it could be).

In any case, I am uncomfortable with the idea to have a scene checker for Python in the core. Somehow, it should be in SofaPython3.

Another case to support is simpleapi :)

@alxbilger alxbilger added pr: enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Jun 15, 2022
@EulalieCoevoet
Copy link
Copy Markdown
Contributor Author

Let's close this PR. I'll do another one with Alex's suggestion when I have time.

@damienmarchal
Copy link
Copy Markdown
Contributor

Too bad, I was just searching for this feature and discovered it was not integrated.

@EulalieCoevoet EulalieCoevoet deleted the pr-missingRequiredPlugins branch April 6, 2023 15:16
@hugtalbot hugtalbot removed the pr: status to review To notify reviewers to review this pull-request label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement About a possible enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants