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

[sysid] Rename quasistatic & dynamic to sweep and step respectively #7748

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

TheComputer314
Copy link
Contributor

@TheComputer314 TheComputer314 commented Jan 29, 2025

if someone wants to rename "quasistatic" and "dynamic" to "sweep" and "step" everywhere i won't complain
it saves typing effort

- Oblarg

Really lazy Ctrl+H, it successfully compiles with all tests passing though.

@TheComputer314 TheComputer314 requested review from a team as code owners January 29, 2025 20:19
Copy link
Contributor

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

@TheComputer314
Copy link
Contributor Author

Merging is definitely a post-season thing due to this change being breaking.

@Oblarg
Copy link
Contributor

Oblarg commented Jan 30, 2025

Because this is breaking, you'll have to add deprecated shims that retain the old names for the robot-code-side classes/methods.

Copy link
Member

@PeterJohnson PeterJohnson left a comment

Choose a reason for hiding this comment

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

The sysid side code needs to be updated to accept logs with tests named either way (so a newer sysid can process a log generated with older code).

In general I'm not sure of the value of making this change in-season. It feels pretty risky in terms of breakage for a very small name improvement.

@TheComputer314
Copy link
Contributor Author

TheComputer314 commented Feb 3, 2025

In general I'm not sure of the value of making this change in-season. It feels pretty risky in terms of breakage for a very small name improvement.

As stated previously, merging will need to be a post-season thing due to being breaking.

@auscompgeek auscompgeek added this to the 2026 milestone Feb 4, 2025
@ArchdukeTim
Copy link
Contributor

It might be better to just rename the enums, but keep the underlying logged names the same, so that compatibility with older logs doesn't break in sysid? Though some people pre-load their logs in AdvantageScope to ensure their data is good, and having the quasistatic/dynamic names there might be confusing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants