chore(validator): enforcing commit message character limit to 50. #25
Conversation
|
Can you ensure the status checks are passing and then mark it "ready for review"? |
|
It is passing the CI Test Suite, but failing the Code Linting check because only a maximum of 88 characters are allowed per statement. |
|
Yes, so refactor your code to comply with the linting rules? The rules has been enforced in a standardised manner in the CI for a specific reason and I do not see any reason to not follow them. |
| * - l (*string): A pointer to the string representing the length | ||
| * of the commit message. |
There was a problem hiding this comment.
The parameter name l is misleading, how about something like msg since it is synonymous to "commit message" which this function should be checking for its length? Update the docs once you have made the necessary changes as well.
| @@ -142,6 +163,11 @@ func ValidateMessage(message *parser.Message) (string, error) { | |||
| return "", fmt.Errorf("%s", err) | |||
| } | |||
|
|
|||
| // Validate the commit message length | |||
| if err := checkMessageLength(&message.Description); err != nil { | |||
There was a problem hiding this comment.
You are supposed to check the entire "commit message" and not just the description of the message.
A commit message is structured as - <TYPE>[OPTIONAL SCOPE]: <DESCRIPTION> (see the summary of the Conventional Commits specifications). An example commit message is - "chore(parser): improve parsing performance of the message".
| // TestCheckMessageLength tests the checkMessageLength function. | ||
| func TestCheckMessageLength(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| message string | ||
| expectedError string | ||
| }{ | ||
| { | ||
| name: "Valid Length", | ||
| message: "feat: add a new feature", | ||
| expectedError: "", | ||
| }, | ||
| { | ||
| name: "Invalid Length", | ||
| message: "Add a new feature that makes the application run " + | ||
| "faster and more efficiently", | ||
| expectedError: fmt.Sprintf( | ||
| "commit message exceeds 50 characters, current length: %d", | ||
| len("Add a new feature that makes the application run "+ | ||
| "faster and more efficiently"), | ||
| ), | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| err := checkMessageLength(&tc.message) | ||
| if tc.expectedError == "" { | ||
| if err != nil { | ||
| t.Errorf("expected no error, got %v", err) | ||
| } | ||
| } else { | ||
| if err == nil || err.Error() != tc.expectedError { | ||
| t.Errorf("expected error %q, got %q", tc.expectedError, err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Excluding this particular, all the other tests are not unnecessary addition in context to the PR. Try to maintain good coding habits and keep the contents of the PR in accordance with the actual context. Everything else can be added little by little in future smaller PRs.
| testCases := []struct { | ||
| name string | ||
| message string | ||
| expectedError string |
There was a problem hiding this comment.
The expectedError can be of the error interface type instead of the generic string interface type no?
| name: "Invalid Length", | ||
| message: "Add a new feature that makes the application run " + | ||
| "faster and more efficiently", | ||
| expectedError: fmt.Sprintf( |
There was a problem hiding this comment.
If the expectedError is of the error interface type then you can use the fmt.Errorf() function to define an error message as well.
| if err == nil || err.Error() != tc.expectedError { | ||
| t.Errorf("expected error %q, got %q", tc.expectedError, err) | ||
| } |
There was a problem hiding this comment.
The conditional check for the error message can be much simplified if the expectedError is of the error interface type, so refactor it after a thorough evaluation of the aforementioned comments.
Description
Implemented checkMessageLength function to enforce the character limit of the commit message to 50.
Changes Include
This commit fixes #24