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

[SofaGraphComponent] Fix a typo in the warning emited by the APIVersion component and add missing allowed versions. #2103

Merged
merged 2 commits into from May 26, 2021

Conversation

jnbrunet
Copy link
Contributor

Using this in your scene:

<APIVersion level="21.06" />

gave (before this PR):

[WARNING] [APIVersion(APIVersion)] The provided level '21.06' is now valid.

and now gives (with this PR);

[WARNING] [APIVersion(APIVersion)] The provided level '21.06' is not valid. Allowed versions are [17.06, 17.12, 18.06, 18.12, 19.06, 19.12, 20.06, 20.12, 20.12.99].

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).

Copy link
Contributor

@alxbilger alxbilger left a comment

Choose a reason for hiding this comment

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

I am just concerned about what happens if the provided level is invalid. The rest of my comments are just ultra-minor suggestions.

auto allowedVersionStr = std::accumulate(std::next(allowedVersion.begin()), allowedVersion.end(), *(allowedVersion.begin()), [](const std::string & s, const std::string & v) {
return s + ", " + v;
});
msg_warning() << "The provided level '"<< d_level.getValue() <<"' is not valid. Allowed versions are [" << allowedVersionStr << "]." ;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens next if the version is invalid? Wouldn't it be better to set a default value (e.g. SOFA_VERSION_STR)?

Copy link
Contributor Author

@jnbrunet jnbrunet May 20, 2021

Choose a reason for hiding this comment

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

Nothing will happen. The purpose of this component is not super clear, but if I understood correctly (maybe @hugtalbot or @guparan could correct me if I'm wrong):

  • Adding <APIVersion level="21.06" /> in your scene is simply used to specify to the user that this scene was written using SOFA v21.06.

Hence, the message emitted here warns the user author of the scene that he probably didn't meant v21.06 since this version does not exists yet (the largest API version is 20.12.99).

Now, we could probably talk about this version number thing at the next dev meeting (cc @hugtalbot for the agenda!) . I often see in other projects the master branch having the version number of the next upcoming release. For example, for the next release, it could look like this:

  1. master branch has version 21.06
  2. Close to the release, a v21.06 branch is created and an alpha or beta binary release is created from this branch
  3. master branch version is updated to 21.12 (even if 21.06 is not released yet)

Using this, setting <APIVersion level="21.06" /> in scene files of the master branch would make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, we could probably talk about this version number thing at the next dev meeting (cc @hugtalbot for the agenda!) . I often see in other projects the master branch having the version number of the next upcoming release. For example, for the next release, it could look like this:

1. `master` branch has version 21.06

2. Close to the release, a `v21.06` branch is created and an alpha or beta binary release is created from this branch

3. `master` branch version is updated to 21.12 (even if 21.06 is not released yet)

Using this, setting <APIVersion level="21.06" /> in scene files of the master branch would make sense.

You are right, the project version was wrong. I used this opportunity to fix it 👍

@jnbrunet jnbrunet added the pr: status to review To notify reviewers to review this pull-request label May 20, 2021
…ion component and add missing allowed versions.
@hugtalbot
Copy link
Contributor

Let's get this merged, but I still do not understand the big picture of this component. It is not spread over SOFA and there is no policy about it.

@hugtalbot hugtalbot 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 May 26, 2021
@epernod epernod added the pr: fix Fix a bug label May 26, 2021
@epernod epernod merged commit 14d4124 into sofa-framework:master May 26, 2021
@guparan guparan added this to the v21.06 milestone Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants