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

Create example subsystem #32

Merged
merged 24 commits into from
Dec 2, 2023
Merged

Conversation

HoodieRocks
Copy link
Contributor

@HoodieRocks HoodieRocks commented Nov 22, 2023

Description

This pull request adds an example subsystem for students to use to learn about how subsystems and commands are used in a robot environment.

Fixes #18

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation, if any
  • My changes generate no new warnings
  • I have performed tests that prove my fix is effective or that my feature works, if necessary
  • New and existing unit tests pass locally with my changes

@HoodieRocks HoodieRocks added the enhancement New feature or request label Nov 22, 2023
@HoodieRocks HoodieRocks self-assigned this Nov 22, 2023
@HoodieRocks
Copy link
Contributor Author

HoodieRocks commented Nov 22, 2023

I realized after making the first commit that I committed the entire thing in one commit. Next time I plan to make smaller commits.

I believe the formatting failed because it expected the JDK 11 and below style case statements.

@HoodieRocks HoodieRocks linked an issue Nov 22, 2023 that may be closed by this pull request
@IanTapply22
Copy link
Member

I realized after making the first commit that I committed the entire thing in one commit. Next time I plan to make smaller commits.

I believe the formatting failed because it expected the JDK 11 and below style case statements.

Nope, it should expect the Java 17 style case statements :)

Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

Just made a couple of reviews regarding comments and some nitpicks at your code. Be sure to resolve these before requesting another round of reviews please.

src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
@IanTapply22 IanTapply22 changed the title Create Example Subsystem Create example subsystem Nov 22, 2023
Copy link
Member

@colemacphail colemacphail left a comment

Choose a reason for hiding this comment

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

Left a few comments. Generally solid, mostly code quality issues I'm spotting. Good job!

Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

Please use this when referencing the intake motor variable.

src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

Be sure to use this with referencing the PDP :)

src/main/java/frc/robot/subsystems/Intake.java Outdated Show resolved Hide resolved
HoodieRocks and others added 2 commits November 22, 2023 14:57
Co-authored-by: Ian Tapply <contact@ian-tapply.me>
Co-authored-by: Ian Tapply <contact@ian-tapply.me>
Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

Just some little nit picks. Wait for @colemacphail's review and I want to go over it again after he does. Don't merge yet :)

Co-authored-by: Ian Tapply <contact@ian-tapply.me>
Copy link
Member

@colemacphail colemacphail left a comment

Choose a reason for hiding this comment

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

1 nit and 2 questions about whether some code is right. Other than Ian's comments and failing CI, lgtm

@IanTapply22
Copy link
Member

Resolve the above and I'll take one last look and give my stamp of approval!

Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

I would recommend on top of my suggestions here just going through the comments and see if they make sense. I just feel like some of the wording for these just feels a little bit off and doesn't really make sense in relation to what the code is doing.

HoodieRocks and others added 3 commits November 22, 2023 21:32
Co-authored-by: Ian Tapply <contact@ian-tapply.me>
Co-authored-by: Ian Tapply <contact@ian-tapply.me>
Co-authored-by: Ian Tapply <contact@ian-tapply.me>
@IanTapply22
Copy link
Member

IanTapply22 commented Nov 23, 2023

Just another thing I'm noticing as well, be sure that when you're writing a commit message you use the following conventions. The main thing is to use a capital letter for every first letter in the first word in a commit message and to be brief and concise.

This project will be going public once everything is finalized and I want to make sure that we're professional with everything as much as possible.

https://www.gitkraken.com/learn/git/best-practices/git-commit-message
https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53

@HoodieRocks
Copy link
Contributor Author

I didn't see this message until after I pushed this commit. I'll keep this in mind next time!

Copy link
Member

@IanTapply22 IanTapply22 left a comment

Choose a reason for hiding this comment

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

LGTM! Just gonna wait for @colemacphail to approve though.

Good job on this subtask!

@IanTapply22
Copy link
Member

@colemacphail do we want to merge this now? The branch is updated and everything looks resolved and good to go. I saw some talk about a different strategy for the intake so I'm just unsure what's happening here.

@colemacphail
Copy link
Member

Just another thing I'm noticing as well, be sure that when you're writing a commit message you use the following conventions. The main thing is to use a capital letter for every first letter in the first word in a commit message and to be brief and concise.

This project will be going public once everything is finalized and I want to make sure that we're professional with everything as much as possible.

https://www.gitkraken.com/learn/git/best-practices/git-commit-message https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53

Gonna point out that this doesn't really matter since the commit messages on main will be the PR description

That being said, keeping commit messages concise and accurate can help in cases like if you need to roll back a commit or look at what changes were made to solve what problem (both maybe more common than you'd think), so you should still be writing good commit messages.

public Command intakeScoreCommand(IntakeScoreType type, IntakeGamepieces expectedPiece) {
return run(() -> {

switch (type) {
Copy link
Member

Choose a reason for hiding this comment

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

fyi @IanTapply22 @HoodieRocks

Not gonna say we need to do this (especially not now, I want this PR in and project relatively done), but I saw this video recently and thought it was cool. Might be relevant here if we want to decouple this behaviour from the subsystems and make things a little clearer.

More reading if you're interested
Java-specific example
(That website saved me in my fourth-year software architecture courses btw -- strongly recommend skimming some of the design patterns if you have time)

@colemacphail colemacphail merged commit 73ea7df into 2024-beta Dec 2, 2023
1 check passed
@colemacphail colemacphail deleted the 18/create-example-subsystem branch December 2, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an example subsystem
3 participants