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.

Sign up for GitHub

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

Jump to bottom

[wpimath] Remove unit suffixes from variable and function names #7529

Open
wants to merge 1 commit into
base: 2027
Choose a base branch
from
Open

[wpimath] Remove unit suffixes from variable and function names #7529

wants to merge 1 commit into from
+2,911 −3,557

Conversation

calcmogul
Copy link
Member

  • Move units into API docs instead because suffixes make user code verbose and hard to read
  • Rename trackWidth to trackwidth
  • Make ultrasonic classes use meters instead of a mix of m, cm, mm, ft, and inches

@calcmogul calcmogul requested review from a team as code owners December 9, 2024 00:19
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@github-actions github-actions bot added component: cscore CameraServer library component: wpilibj WPILib Java component: wpilibc WPILib C++ component: hal Hardware Abstraction Layer component: command-based WPILib Command Based Library component: wpimath Math library component: examples component: sysid SysId app 2027 2027 target labels Dec 9, 2024
@calcmogul calcmogul force-pushed the wpimath-remove-unit-suffixes-from-variable-names branch 4 times, most recently from 6046a8f to 5326e0f Compare December 9, 2024 00:45
KangarooKoala
KangarooKoala reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Love to see how much cleaner wpimath is now! However, I noticed that some overloads that use the Java units system got the units added to their doc comments. (e.g., new ChassisSpeeds(LinearVelocity, LinearVelocity, AngularVelocity), DifferentialDriveOdometry.resetPosition(Rotation2d, Distance, Distance, Pose2d), and many others). Are the doc comments necessary? Functions in C++, which all use the units system, don't include the units in the doc comments.

Also, just so we don't forget to include this in the change notes, this changes the DifferentialDriveKinematics schema.

wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/ElevatorSim.java Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/FlywheelSim.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

For getAngularVelocity() and getAngularAcceleration(), was the plan to return units or doubles?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't do both with the same name, so I was planning on just using doubles.

wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/LinearSystemSim.java Outdated Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/SingleJointedArmSim.java Outdated Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/SingleJointedArmSim.java Outdated Show resolved Hide resolved
wpimath/src/main/java/edu/wpi/first/math/estimator/KalmanTypeFilter.java Outdated Show resolved Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisSpeeds.java Outdated Show resolved Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/DifferentialDriveKinematics.java Outdated Show resolved Hide resolved
wpimath/src/main/java/edu/wpi/first/math/system/plant/DCMotor.java Outdated Show resolved Hide resolved
wpimath/src/main/java/edu/wpi/first/math/trajectory/Trajectory.java Outdated Show resolved Hide resolved
@calcmogul calcmogul changed the title [wpimath] Remove unit suffixes from variable names [wpimath] Remove unit suffixes from variable and function names Dec 10, 2024
KangarooKoala
KangarooKoala reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

The differential drive odometry and pose estimators still have changed @param comments for units parameters.

wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/ElevatorSim.java Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/ElevatorSim.java Show resolved Hide resolved
@calcmogul calcmogul force-pushed the wpimath-remove-unit-suffixes-from-variable-names branch 5 times, most recently from aebeb04 to 313181f Compare December 14, 2024 20:45
@calcmogul
[wpimath] Remove unit suffixes from variable names
6fc2306 
* Move units into API docs instead because suffixes make user code verbose and hard to read
* Rename trackWidth to trackwidth
* Make ultrasonic classes use meters instead of a mix of m, cm, mm, ft,
  and inches
@calcmogul calcmogul force-pushed the wpimath-remove-unit-suffixes-from-variable-names branch from 313181f to 6fc2306 Compare December 16, 2024 01:47
KangarooKoala
KangarooKoala reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Looks good to me, for what it's worth. I'll just note that merging this would mean every time we merge main back into 2027, any changes to wpimath will need to fix the merge conflicts. (The changes should be fairly trivial, though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KangarooKoala KangarooKoala KangarooKoala left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
2027 2027 target component: command-based WPILib Command Based Library component: cscore CameraServer library component: examples component: hal Hardware Abstraction Layer component: sysid SysId app component: wpilibc WPILib C++ component: wpilibj WPILib Java component: wpimath Math library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@calcmogul @KangarooKoala