-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Warn when fixing enhancements without changelog #274
base: master
Are you sure you want to change the base?
Conversation
Necessary in order to allow us to mock diffs.
Diffs are served via github.com, not api.github.com.
Thanks for your pull request, @CyberShadow! |
@@ -14,6 +14,27 @@ struct UserMessage | |||
} | |||
|
|||
|
|||
void checkEnhancementChangelog(in ref PullRequest pr, ref UserMessage[] msgs, |
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.
Don't use in ref
please
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.
I'm using the same signature as the method below.
source/dlangbot/warnings.d
Outdated
if (!diff.canFind("\n+++ b/changelog/")) | ||
{ | ||
msgs ~= UserMessage(UserMessage.Type.Warning, | ||
"Pull requests implementing enhancements should include a [full changelog entry](https://github.com/dlang/dmd/tree/master/changelog)." |
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.
Wait, won't this trigger for other repos too ?
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.
As of #273 the list of refs for non-Bugzilla-using projects should be empty, which should match projects which use the changelog
directory.
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.
Oh I see what you mean, well spotted. Fix pushed.
Fixes #178.