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

Commenting step 1 #39

Open
wants to merge 7 commits into
base: Dev
Choose a base branch
from
Open

Commenting step 1 #39

wants to merge 7 commits into from

Conversation

jforchheimer25
Copy link
Member

Pull Request for Commenting Branch

@jforchheimer25 jforchheimer25 requested a review from dlezcan1 April 8, 2024 22:29
@jforchheimer25 jforchheimer25 self-assigned this Apr 8, 2024
Copy link
Member

@dlezcan1 dlezcan1 left a 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.

Comment on lines +157 to +161

/** <b> DETAILED EXPLANATION </b> */
public static int Explanation() {
return 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

@dlezcan1 dlezcan1 Apr 8, 2024

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

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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 {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

want*

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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
Copy link
Member

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)

Copy link
Member Author

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

Copy link
Member

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.

@dlezcan1 dlezcan1 linked an issue Apr 8, 2024 that may be closed by this pull request
@dlezcan1
Copy link
Member

dlezcan1 commented Apr 8, 2024

@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

@dlezcan1
Copy link
Member

dlezcan1 commented Apr 8, 2024

When people want explanation for things I would like for everything to be in the same place

@jforchheimer25
Then you can do that 1 of two ways. 1: A separate .java file with comment block descriptions, or a README.md file which connects all of these together. A class with arbitrary functions which return arbitrary integers is confusing and clunky. It's sleeker to stick to comment blocking

@dlezcan1
Copy link
Member

dlezcan1 commented Apr 8, 2024

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

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

@jforchheimer25
Copy link
Member Author

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?

@dlezcan1
Copy link
Member

dlezcan1 commented Apr 8, 2024

Here is an example for Python of good structured commenting:

https://github.com/PhotonVision/photonvision/blob/master/photon-lib/py/photonlibpy/photonPoseEstimator.py

@jforchheimer25
Copy link
Member Author

Here is an example for Python of good structured commenting:

https://github.com/PhotonVision/photonvision/blob/master/photon-lib/py/photonlibpy/photonPoseEstimator.py

Ill Remove the class later then. Im more focused on getting the explanations in first.

@dlezcan1
Copy link
Member

dlezcan1 commented Apr 8, 2024

Sure, but keep new additions to be class-based and function-based. No need for the Explanation functions.

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

Successfully merging this pull request may close these issues.

Documentation of Code
2 participants