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

updated python binding names in all pytests #3005

Closed
wants to merge 4 commits into from

Conversation

AadityaSuri
Copy link
Contributor

@AadityaSuri AadityaSuri commented Oct 2, 2023

Description

updated python binding names in all tests from xx -> xx_cpp, ex: tbots -> tbots_cpp

Testing Done

ran all simulatedTests and other tests, all running well

Resolved Issues

Update python binding import alias #2989

@AadityaSuri AadityaSuri self-assigned this Oct 2, 2023
Remove all suffixes in FSM diagram guards (UBC-Thunderbots#2976)
Copy link
Contributor

@nimazareian nimazareian left a comment

Choose a reason for hiding this comment

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

Good job with making this change! Just had a couple of nits.
Also, in the future I recommend not working directly off of your fork's master branch, and instead creating a new branch from the upstream/master branch for every ticket you work on. i.e.

# Go to the main repo's (upstream) master branch and pull its latest version 
git checkout upstream/master
git pull upstream master
# Create a new branch under your own fork for the specific task youre working on
git checkout -b <branch_name_related_to_ticket>

Also, I noticed a couple of places where we do something like from software.python_bindings import *. For consistency, can we also make it so we also use the tbots_cpp namespace there as well. E.g. Note that in this case anything that's imported (such as SSLRefereeProtoListener) will then need to have the prefix.

from software.python_bindings import *

This should also be updated:

import software.python_bindings as cpp_bindings

@@ -1,4 +1,4 @@
import software.python_bindings as geom
import software.python_bindings as geom_cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

If this import is not being used in this file, we should remove it

import software.python_bindings as geom
import software.python_bindings as geom_cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets keep the naming consistent as tbots_cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review, ill make the all the suggested changes.

@nimazareian
Copy link
Contributor

Also, I recommend updating the PR description to say "resolves #2989" so the ticket is automatically closed once you merge your PR! More keywords can be found here: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@nimazareian
Copy link
Contributor

Closing in favour of #3021

@nimazareian nimazareian closed this Oct 8, 2023
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.

2 participants