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

Change Modifications to be its own class #9

Merged
merged 7 commits into from
Feb 10, 2022

Conversation

GrmanRodriguez
Copy link
Contributor

This PR changes the way we write modifications in the urdf-modifiers library.

Modification is now a class part of urdfModifiers.core with methods for adding the different modifications.

A constructor for creating this class from a section of an ini file has also been added.

Below some examples of the changes:

Previous way of creating modifications:

modifications= {}
modifications[utils.geometry.Modification.DIMENSION] = [0.2, utils.geometry.Modification.ABSOLUTE] 
modifications[utils.geometry.Modification.DENSITY] = [2.0, utils.geometry.Modification.MULTIPLIER]

New method:

modifications = Modification()
modifications.add_dimension(0.2, absolute=True)
modifications.add_density(2.0, absolute=False)

As you can see it is now clearer and the code is cleaner.

cc @CarlottaSartore

@traversaro
Copy link
Member

fyi @fabiodinatale

Copy link

@vvasco vvasco left a comment

Choose a reason for hiding this comment

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

LGTM!

@traversaro
Copy link
Member

@GrmanRodriguez @vvasco can we tag the version of the library before this PR as v0.0.1 or something similar? we have some scripts that use the old modifications API, and while we could refer to specific checkout, it may be easier to refer to a given tag. No release is necessary, just a small tag is ok.

@vvasco
Copy link

vvasco commented Feb 8, 2022

good idea @traversaro thanks

@vvasco
Copy link

vvasco commented Feb 8, 2022

I'm creating a tag v0.0.1

@GrmanRodriguez
Copy link
Contributor Author

In that case I'd say first accept #12 since it's non-breaking and adding it to the tag

@vvasco
Copy link

vvasco commented Feb 8, 2022

In that case I'd say first accept #12 since it's non-breaking and adding it to the tag

I agree. Then after that PR is accepted and merged, we can proceed with the tag.

Copy link
Contributor

@AlexAntn AlexAntn left a comment

Choose a reason for hiding this comment

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

LGTM!

PR #12 has been merged, so we can create the tag and then merge this PR too.

Great work @GrmanRodriguez !

@AlexAntn AlexAntn merged commit 0cca967 into master Feb 10, 2022
@AlexAntn AlexAntn deleted the feat/modificationsAsClass branch February 10, 2022 12:06
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.

5 participants