-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: rename pos vel acc #200
Conversation
7bc8808
to
de78b7a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=======================================
Coverage 95.92% 95.92%
=======================================
Files 78 78
Lines 2946 2946
=======================================
Hits 2826 2826
Misses 120 120 ☔ View full report in Codecov by Sentry. |
a3e033c
to
609b8a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not strongly opposed or in favor of this, but want to talk it through. One other place where words could be shortened is in the prefix (Cart/Sph/Cyl/etc.). I'm curious to hear if you have any thoughts about pros/cons to abbreviating the prefix instead of pos/vel/acc, or both, or neither.
src/coordinax/_src/base/__init__.py
Outdated
# Pos | ||
"AbstractPos", | ||
# Vel | ||
"AbstractVel", | ||
# Acc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Total nit pick, but comments should probably stay as full words Position, Velocity, Acceleration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Product of global find & replace
I'm not opposed to shortening the prefix either / instead. The goal was to see if it's worth shortening the otherwise long names. I'm still not sure shortening is a good idea 😅. |
f51ca49
to
e7d425b
Compare
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
c6bafb5
to
b27e9a1
Compare
We discussed this in person and decided to move forward with the name shortening. |
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Shorten the names of most classes. Meaning still obvious.
When we add docs (hopefully soon) this will go into the conventions page.
@adrn what do you think?