-
-
Notifications
You must be signed in to change notification settings - Fork 117
Possibly add commit linting for core commits #54
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
Comments
Any thoughts on how this should look like in a PR? A couple of options comes to mind:
The benefit of option 2 is that we can actually print all the messages your core-validate-commit module generates, that's not as easy with option 1 as we can only display one line of text IIRC. ..or maybe a combination of both would be ideal? |
2 can be spammy. The status api can provide a message as well -- if users push updates to pr and expect the bot to give it new info about linting it would probably annoy a lot of people that get notifications on issue updates. |
Also, since we can link away to $url, we can notify users of the issues over there. |
@jbergstroem, one way to avoid spamming people is to have the bot update an existing comment. My approach on other things was to have the bot append to the author's issue message. |
@williamkapke ok -- not sure if that sends another email or not. How would the author know its updated? Edit: Ok, so, editing the top message. Just so i better understand all options: what are the downsides of using the build status api? |
I do think it would be nice to be able to update whenever a push is made. I am not sure the best path to go though. I was planning to do some proof of concepts with both and see which one we like better? |
@evanlucas sounds good to me. |
We're not able to display much of the errors and warnings the core-validate-commit module generates, as GH truncates status descriptions. As an example I just tried to set the following description on a PR: Line #20: Fixes must be a url, not an issue number. which ended up looking like this: To overcome that we could provide an URL with the status which will make the "Details" link appear, but that means we'll have to create an endpoint somewhere which renders the complete list of linting issues for a commit - IMO it smells too complex for a first shot at commit msg linting.
Have a look at scripts/display-travis-status.js#L10 where we listen for PR updates. And creating/updating a status can be done with the {
user: 'phillipj',
repo: 'webhooks-testing',
sha: 'dff8fcf880017cd212cdc5c28931420dd1988201',
context: 'testing-statuses',
state: 'failure',
description: 'Line #20: Fixes must be a url, not an issue number.\nLine #3: Line should be <= 72 columns.'
} |
Should we still do this? @jbergstroem has expressed some concern about commits not being ready for linting until just before landing, therefore displaying an inline PR status before then isn't that valuable? |
I think this could still be useful, as long as we don't require people to have this green before someone lands it. Related: nodejs/node#12107 (cc/ @vsemozhetbyt) |
Moving this to nodejs/build#793 |
I have been working on using core-validate-commit to lint a commit based on a github patch. Maybe this is something we could integrate into the bot?
/cc @jbergstroem
The text was updated successfully, but these errors were encountered: