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

Python interface: Renames methods to match PEP8 style #226

Merged
merged 4 commits into from
Aug 19, 2021

Conversation

francocipollone
Copy link
Contributor

🎉 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

@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

codecov bot commented Aug 19, 2021

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.

Copy link
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
Contributor Author

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
Member

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
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants