-
Notifications
You must be signed in to change notification settings - Fork 113
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
add class-level comments #144
Conversation
@@ -2,6 +2,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
module Packwerk | |||
# A command-line interface to Packwerk. |
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.
This one feels a little too much like describe the class name
and I don't think it adds any value.
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 was on the fence about this one, but then thought - does everyone know CLI means command-line interface?
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 have the same doubts as me I should probably remove this one.
class Graph | ||
# @param [Array<Array>] edges The edges of the graph; An edge being represented as an Array of two nodes. |
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.
Should be we using sorbet?
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, we could use sorbet, that's true - I wanted to keep this PR to comments only so we could more easily merge it. 🤔
Sorbet would make it way more explicit and also be guaranteed to match the code, so definitely better. Will think about doing it later, want to play around with restructuring the code first in a separate PR
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.
Even if we were using sorbet we should be documenting our methods and parameters.
There are a few other classes that should probably get a comment, like |
class Graph | ||
# @param [Array<Array>] edges The edges of the graph; An edge being represented as an Array of two nodes. |
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.
Even if we were using sorbet we should be documenting our methods and parameters.
What are you trying to accomplish?
Make packwerk code easier to understand to enable easier maintenance.
What approach did you choose and why?
Add class / module level comments in a general way that shouldn't become outdated anytime soon.
At this point we have a lot of classes and modules directly in the
Packwerk
namespace, and it's absolutely not obvious what each of them does without reading the implementation of multiple of them.Even for me who has been at least superficially involved with most of the development of the library, this lack of documentation creates a lot of friction.
What should reviewers focus on?
Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist