Skip to content

Python interface: Renames methods to match PEP8 style#226

Merged
scpeters merged 4 commits intoign-math6from
francocipollone/rename_python_interface
Aug 19, 2021
Merged

Python interface: Renames methods to match PEP8 style#226
scpeters merged 4 commits intoign-math6from
francocipollone/rename_python_interface

Conversation

@francocipollone
Copy link
Copy Markdown
Contributor

@francocipollone francocipollone commented Aug 19, 2021

🎉 New feature

Related to #101 #210

Summary

  • Python interfaces for Vector2, Vector3,Vector4,Angle,GaussMarkovProcess and Rand classes to match PEP8 style.
  • Use rename SWIG directives to rename methods when creating the python bindings. (Aligned with PEP8 specification.)
  • Python and Ruby files were moved to different folders in order to have for each language different SWIG files given that between languages probably the .i could change a bit.
    • For example: For the particular case of renaming, for Python we need to rename the methods but that's not the case for Ruby.

For reviewers

I suggest to review commit by commit:

  • Use separate folder for ruby and python swig files and tests … --> c0e8e6d
  • Renames python interface to match PEP8 style … --> a419f3c
  • Update python examples … --> c54755f

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@francocipollone
Use separate folder for ruby and python swig files and tests
c0e8e6d 
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Renames python interface to match PEP8 style
a419f3c 
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Update python examples
c54755f 
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Aug 19, 2021
@github-actions github-actions bot added 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 19, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 19, 2021 •
edited
Loading

Codecov Report

Merging #226 (dc50c48) into ign-math6 (cf5e33c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #226   +/-   ##
==========================================
  Coverage      99.21%   99.21%           
==========================================
  Files             65       65           
  Lines           6089     6089           
==========================================
  Hits            6041     6041           
  Misses            48       48

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf5e33c...dc50c48. Read the comment docs.

LolaSegura
LolaSegura approved these changes Aug 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@LolaSegura LolaSegura left a comment

Choose a reason for hiding this comment

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

Nice job, LGTM!

@francocipollone
Copy link
Copy Markdown
Contributor Author

francocipollone commented Aug 19, 2021

We could use one more review @chapulina @scpeters . 😃 🙏
Even though there are many lines in the git diff, the real changes aren't so many. Reviewing commit by commit is the suggestion.

Once this PR gets merged, the open PRs that add python interfaces for more classes should be updated. CC: @LolaSegura @WagnerMarcos

@scpeters
Copy link
Copy Markdown
Member

scpeters commented Aug 19, 2021

I tried running pylint on the generated ignition/math.py file and it still complains about method names that are fewer than 3 characters in length. The following are complaints about the Vector4f class:

C:1108, 4: Method name "x" doesn't conform to snake_case naming style (invalid-name)
C:1111, 4: Method name "y" doesn't conform to snake_case naming style (invalid-name)
C:1114, 4: Method name "z" doesn't conform to snake_case naming style (invalid-name)
C:1117, 4: Method name "w" doesn't conform to snake_case naming style (invalid-name)

I think we can ignore this though

@scpeters scpeters merged commit 86dc7a8 into ign-math6 Aug 19, 2021
@scpeters scpeters deleted the francocipollone/rename_python_interface branch August 19, 2021 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters scpeters approved these changes

@chapulina chapulina Awaiting requested review from chapulina

+1 more reviewer

@LolaSegura LolaSegura LolaSegura approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@francocipollone @scpeters @LolaSegura @j-rivero @chapulina @osrf-triage