-
Notifications
You must be signed in to change notification settings - Fork 1
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
Commenting step 1 #39
base: Dev
Are you sure you want to change the base?
Conversation
…iseHangAuto and LowerHangAuto Commands.
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 do not directly understand what this Explanations static class is supposed to do and what the "Explanation" functions are supposed to do.
|
||
/** <b> DETAILED EXPLANATION </b> */ | ||
public static int Explanation() { | ||
return 1; | ||
} |
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.
What is this?
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 Created a function called explanation in each class. in the explanation file people will get a clear explanation of how things work based off of the java doc of that explanation
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.
Why is there a function rather a direct comment block with the class and comment blocks for each of the functions? Why use this arbitrary Explanation
function?
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 didnt want to make the explanation for the class to long
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 think you should focus more on comment blocks, rather than this Explanation
class system. I would revert these commits and go with comment blocking
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.
Also We need to have explanations for things like init Sendable and command schedule and auto etc
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.
Those can be done for the initSendable
functions, individually
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.
What about command Scheduler etc. I want to make it so that each student who wants to code after me knows exactly where to look for stuff. I cant java doc the init sendable stuff really and command scheduler stuff
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.
Take this to the main comment block. I'll respond there.
*/ | ||
public class Explanations { | ||
|
||
public abstract class SubsystemExplanations { |
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.
Why is the public abstract class?
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.
Dont wont to make an object of 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.
Why make an object out of it at all? Why is this a class?
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.
want*
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.
Right, why is this a class at all?
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 wanted to make it clear for younger students where all the explanataions would be
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 would argue that the class implementation is more confusing itself. I think this would be more smoothly implemented with your structure you have in Hanger.java
minus the Explanation
function.
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.
When people want explanation for things I would like for everything to be in the same place
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.
If you look in Hanger Module The explanation is like a good 200 words that I dont want to be shown when hovering over HangerModule.java
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.
Break the comments into their individual functions then.
@@ -292,7 +297,7 @@ public void driveChassisSpeeds(ChassisSpeeds chassisSpeed) { | |||
/** | |||
* Resets the Position of the Odometer, given our Current position. | |||
* | |||
* @param Pose2d (pose2d) - The current position of the robot on the field. This is a {@link | |||
* @param pose2d (pose2d) - The current position of the robot on the field. This is a {@link |
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.
Typical format is param name (Parameter's Class). change pose2d (pose2d)
to pose2d (Pose2d)
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 intended to do that but misclicked apparently
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.
Got it. Will resolve comment when addressed.
@jforchheimer25 Thread here: I believe the structure of an abstract static class is more confusing than just using standard comment blocks for all functions and classes |
@jforchheimer25 |
This can be commented where it is implemented. We could have a separate function on each and link them to the web pages with our simpler explanation |
could you show an example? |
Here is an example for Python of good structured commenting: |
Ill Remove the class later then. Im more focused on getting the explanations in first. |
Sure, but keep new additions to be class-based and function-based. No need for the |
Pull Request for Commenting Branch