formal: refactor and check for more guideline violations #64
Conversation
GeorgeSapkin
force-pushed
the
formal-check-for-more-guideline-violations
branch
from
e710b0f to
6e78b54
Compare
.github/scripts/check_formalities.sh
Outdated
| line_num=$((line_num + 1)) | ||
| if [ ${#line} -gt "$MAX_BODY_LINE_LEN" ]; then | ||
| warn "Commit body line $line_num is longer than $MAX_BODY_LINE_LEN characters (is ${#line}):" | ||
| split_err "$MAX_BODY_LINE_LEN" "$line" |
There was a problem hiding this comment.
I am not sure, if we want to be such strict for commit description, because something I put there what it fixes, which error I've experienced and that usually contains more than 75 characters on one row. :-(
There was a problem hiding this comment.
This is just a warning and not an error, i.e the job will still pass. Same for subject.
There was a problem hiding this comment.
Oh, you are completely right. I dont know how I missed it. Writing comments after midnight is not always a good idea. :)
.github/scripts/check_formalities.sh
Outdated
|
|
||
| # Check subject length | ||
| if [ ${#subject} -gt "$MAX_SUBJECT_LEN" ]; then | ||
| warn "Commit subject line is longer than $MAX_SUBJECT_LEN characters (is ${#subject})" |
There was a problem hiding this comment.
I understand that the more rules there are, the more complicated it will simply be, but at this point I think that enforcing a rule that the commit subject should be brief, precise, simple and concise is something we want, and that it shouldn't be a warning, but an error.
An example is this pull request: openwrt/packages#27564, the GitHub UI hid that subject because it's simply too long, which should have helped tell the user that it's long, but apparently that didn't happen. Just to be sure, here's a link to the commit itself: openwrt/packages@a5a7ca7
On the other hand, the commit description can be a warning. Wrapping or not wrapping at 75 characters per line doesn't bother me personally.
There was a problem hiding this comment.
Thinking out loud here. Maybe we can add soft and hard limits? Warning at 50, error at 60? Some package names like the one in your example, take more than half of the 50 character budget.
BKPepe
left a comment
BKPepe
left a comment
There was a problem hiding this comment.
This looks good. 👍 Thank you, @GeorgeSapkin
@Ansuel your thoughts?
GeorgeSapkin
force-pushed
the
formal-check-for-more-guideline-violations
branch
from
6e78b54 to
6a1ea72
Compare
Improve readability of CI output by omitting unnecessary columns. Signed-off-by: George Sapkin <[email protected]>
Refactor checks into separate functions to be more maintainable in preparation for posting formality check results to GitHub. Check for more guideline violations based on https://openwrt.org/submitting-patches#submission_guidelines - Check if first word after prefix is capitalized. - Check if subject ends with a period. - Check subject line and body line lengths and colorize the excess. Warn on soft subject length failure (50 chars) and error on hard (60 chars ). Exclude Weblate commits from these checks. Signed-off-by: George Sapkin <[email protected]>
GeorgeSapkin
force-pushed
the
formal-check-for-more-guideline-violations
branch
from
6a1ea72 to
ed92aba
Compare
GeorgeSapkin
changed the title
|
I have refactor checks into separate functions to be more maintainable in preparation for posting formality check results to GitHub (next PR) which will be configured at the repo side (i.e. opt-in) and look something like this: (commit hashes are clickable in an actual PR) Failed checksIssues marked with a stop sign 🛑 are failing checks. Commit 1b3158ac23041c9aa07f08fa17fabe05b336886b
Commit adfcad580fad4828d7227824c824998571d84ea5
Commit fa00178319f058f4ede84c4b5988ef47f06dbb10
Commit 80ef743772929b0ea838fdd35e870da31545fdd3
Commit 24f5ed50b8eb2435f511ac5c68d75e1f40bc31d1
Commit efdd422be6e6254871d212ab22a2650fcc6033e8
Commit d4a18b1085b0c88fb48a22a78edb2554b4a34a7f
Commit eb99d6415a3f1f67f1bd87c32221c10e175cc249
Commit e02f3ea62a864eff7bef9996f18388786c26ce39
Commit 4dc4d27aa5654e8b7c9168b919af105c981d79df
For more details, see the full job log. |
|
@systemcrash can you double-check the Weblate logic, when you have time? I've been looking at this for so long, I probably missed something again 🫎 |
|
weblate looks OK I think. But need it be flagged as 'warn'? |
Not really. My though process is that it's something that's only shown in the job log, and only for Weblate jobs. It's not failing the job and it's not going to be show in the future GitHub comment. So in case somebody abuses this logic in a normal PR (i.e. author with a Weblate email), it's better that it slightly stands out. I don't have strong feeling about it either way. |
|
Fair. |
3 participants
Refactor checks into separate functions to be more maintainable in preparation for posting formality check results to GitHub (next PR).
Check for more guideline violations based on https://openwrt.org/submitting-patches#submission_guidelines
Check if first word after prefix is capitalized (my personal pet peeve). Results in an error.
Check if subject ends with a period. Results in an error as well.
Check subject line and body line lengths and colorize the excess. Warn on soft subject length failure (50 chars) and error on hard (60 chars). Exclude Weblate commits from these checks.
Improve readability of ci_helper output by omitting unnecessary columns.
Example output with various scenarios: https://github.com/GeorgeSapkin/openwrt-packages/actions/runs/19236252019/job/54986863750?pr=1#step:5:1
Related discussion: openwrt/packages#27790