-
Notifications
You must be signed in to change notification settings - Fork 0
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
Change file names to match new conventions #72
base: main
Are you sure you want to change the base?
Conversation
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.
some minor things
src/main/java/frc/robot/subsystems/indexer/IndexerInterface.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/robot/subsystems/elevator/ElevatorInterface.java
Outdated
Show resolved
Hide resolved
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.
plz fix
@AutoLog | ||
public static class ElevatorIOInputs { | ||
public static class ElevatorInterfaceInputs { |
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.
ElevatorInputs
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.
por favor
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.
plz correct this in all da files bc the elevator inputs are autologged from the specific impls. they aren't logged from the interface. @JacksonElia do u think this is good?
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.
yeah I think it is. I really think we should try to avoid using default
and not implement any methods in interfaces as that defeats their purpose. If we really want to implement methods in the interface, we can just make them abstract classes.
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.
okay after looking into it more I guess its not awful using default
. I looked at their template code and some things have it, but where they use it feels icky to me. I'm gonna ask eldest Matthew about it
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.
The reason I think we do that is bc for log replay u need a blank impl of the io layer to rerun the code. So we just pass in the interface. What we could do instead is create a blank impl with the defaults seen in the interface, which would work. Idk which one is better. So like as opposed to using ElevatorInterface
we would use ElevatorReplay
or smthn.
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.
Thats a good explanation, thanks. I would say normally I feel like making something like ElevatorReplay
would be better practice, but in this case I guess following the convention of using default like akit has already specified makes more sense.
Though, if we start writing a lot of replay specific code for whatever reason, then we should make a specific implementation for it.
Change names to match conventions