-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
misc(changelog-generator): Generate changelogs #3632
Conversation
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.
nice @wardpeet thanks for picking this up!
|
||
{{~!-- commit hash --}} {{#if @root.linkReferences}}([{{hash}}]({{#if @root.host}}{{@root.host}}/{{/if}}{{#if @root.owner}}{{@root.owner}}/{{/if}}{{@root.repository}}/{{@root.commit}}/{{hash}})){{else}}{{hash~}}{{/if}} | ||
|
||
{{~!-- commit references --}}{{#if references}}, closes{{~#each references}} {{#if @root.linkReferences}}[{{#if this.owner}}{{this.owner}}/{{/if}}{{this.repository}}#{{this.issue}}]({{#if @root.host}}{{@root.host}}/{{/if}}{{#if this.repository}}{{#if this.owner}}{{this.owner}}/{{/if}}{{this.repository}}{{else}}{{#if @root.owner}}{{@root.owner}}/{{/if}}{{@root.repository}}{{/if}}/{{@root.issue}}/{{this.issue}}){{else}}{{#if this.owner}}{{this.owner}}/{{/if}}{{this.repository}}#{{this.issue}}{{/if}}{{/each}}{{/if}} |
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 is, uh, quite the line 😄
any chance we can break it up and use the fancy whitespace removers ~
?
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.
yes we can, just copy and paste 😊
commit.hash = commit.hash.substring(0, 7); | ||
} | ||
|
||
if (commit.type) { |
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 we also de-dupe test and tests while we're in here?
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.
fine by me, I just copy pasted it 😊
one question do we want to show
|
Hm I personally don't think it's necessary to reference the closed issues when the PR is referenced, but I'm open to others thoughts? |
Nope just the PR is fine for a changelog. Can leave issues out of it.
…On Friday, October 27, 2017, Patrick Hulce ***@***.***> wrote:
Hm I personally don't think it's necessary to reference the closed issues
when the PR is referenced, but I'm open to others thoughts?
@paulirish <https://github.com/paulirish> @brendankenny
<https://github.com/brendankenny> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3632 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACZF8Ynba9TKLYC_G2kvWdC-wEH2bVhks5swktegaJpZM4QBvyA>
.
|
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.
looking good! A few other things:
- Pull the PR # out of the string in
headerPattern
as well? (though still print it as it is now) - Maybe don't include the commit hash if the PR# exists? Not sure how others feel, but I never care about the actual commit over the PR that generated it
- different directory? I don't have a great alternate suggestion, but maybe this could live under
docs/
? Or have a top levelbuild/
directory or something
@@ -0,0 +1,55 @@ | |||
'use strict'; |
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.
needs license header
commitsSort: ['type', 'scope', 'message'], | ||
}; | ||
|
||
const template = readFileSync(resolve(__dirname, 'templates/template.hbs')).toString(); |
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.
maybe move these declarations to top of file so writerOpts.mainTemplate
, etc can just be defined in the object literal?
@@ -0,0 +1,46 @@ | |||
* {{#if message ~}} |
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.
license header for these templates, too (need to convert to handlebars comments, I guess)
package.json
Outdated
@@ -46,14 +46,16 @@ | |||
"smokehouse": "node lighthouse-cli/test/smokehouse/smokehouse.js", | |||
"deploy-viewer": "cd lighthouse-viewer && gulp deploy", | |||
"bundlesize": "bundlesize", | |||
"plots-smoke": "bash plots/test/smoke.sh" | |||
"plots-smoke": "bash plots/test/smoke.sh", | |||
"changelog:generate": "conventional-changelog -n ./conventional-changelog-lighthouse/index.js -i CHANGELOG.md -s" |
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.
not sure how others feel, but I'd prefer just yarn changelog
also, I think I ran into that semver/tagging issue you were talking about. When I run this locally (after bumping to 2.6.0 for testing purposes) it does Did you figure out a fix for that? The |
@brendankenny could it be that you didn't create a tag from master as when I redid the tagging through master all went ok I created the correct tag on github so it should work now if you do a |
9728b6c
to
8e26dfd
Compare
@brendankenny now it shows PRs and no issue references <a name="2.5.1"></a>
# 2.5.1 (2017-11-03)
[Full Changelog](https://github.com/googlechrome/lighthouse/compare/v2.5.0...v2.5.1)
### New audit
* descriptive anchor text audit ([#3490](https://github.com/googlechrome/lighthouse/pull/3490))
### Core
* skip aspect ratio audit for svg ([#3722](https://github.com/googlechrome/lighthouse/pull/3722))
* add category weight to perf config ([#3529](https://github.com/googlechrome/lighthouse/pull/3529))
* add silent seo audits to default config ([#3582](https://github.com/googlechrome/lighthouse/pull/3582))
* Re-weight a11y scores based on severity and frequency ([#3515](https://github.com/googlechrome/lighthouse/pull/3515))
* Remove iframe as Critical Request ([#3583](https://github.com/googlechrome/lighthouse/pull/3583))
* add acyclic check ([#3592](https://github.com/googlechrome/lighthouse/pull/3592))
* fix missing `Runtime.experiments` object ([#3514](https://github.com/googlechrome/lighthouse/pull/3514))
* use execution context isolation when necessary ([#3500](https://github.com/googlechrome/lighthouse/pull/3500))
* add cz-customizable to establish a commit message convention ([#3499](https://github.com/googlechrome/lighthouse/pull/3499))
* Fix minor grammatical error ([#3638](https://github.com/googlechrome/lighthouse/pull/3638))
* remove use of deprecated Emulation.setVisibleSize ([#3536](https://github.com/googlechrome/lighthouse/pull/3536))
* record top-level warnings in LHR and display in report ([#3692](https://github.com/googlechrome/lighthouse/pull/3692))
* include runtime exceptions ([#3494](https://github.com/googlechrome/lighthouse/pull/3494))
* pass audit when no images are missized ([#3552](https://github.com/googlechrome/lighthouse/pull/3552))
* add error reporting ([#2420](https://github.com/googlechrome/lighthouse/pull/2420))
* refactor simulation logic ([#3489](https://github.com/googlechrome/lighthouse/pull/3489))
* add transferSize sanity check ([#3606](https://github.com/googlechrome/lighthouse/pull/3606))
* fix typo in image-aspect-ratio audit ([#3513](https://github.com/googlechrome/lighthouse/pull/3513))
### Docs
* add results object explainer ([#3495](https://github.com/googlechrome/lighthouse/pull/3495))
* Updating "good first bug" ([#3535](https://github.com/googlechrome/lighthouse/pull/3535))
* Updating bug labels ([#3525](https://github.com/googlechrome/lighthouse/pull/3525))
* pr title guidelines ([#3590](https://github.com/googlechrome/lighthouse/pull/3590))
* correct capitalization of GitHub ([#3669](https://github.com/googlechrome/lighthouse/pull/3669))
* add MagicLight WebBLE integration ([#3613](https://github.com/googlechrome/lighthouse/pull/3613))
* add Treo to the list of integrations ([#3484](https://github.com/googlechrome/lighthouse/pull/3484))
* because comcast throttles the websocket([bedb9a1](https://github.com/googlechrome/lighthouse/commit/bedb9a1))
### Feat
* Adding Redirect audit (PSI Compat) ([#3308](https://github.com/googlechrome/lighthouse/pull/3308))
### Misc
* add commitlint config (for commitlintbot)([21e25aa](https://github.com/googlechrome/lighthouse/commit/21e25aa))
* Remove # from pr([01efa91](https://github.com/googlechrome/lighthouse/commit/01efa91))
* Fix template([0d77fa9](https://github.com/googlechrome/lighthouse/commit/0d77fa9))
* Combine test and tests to one category([7b0ac70](https://github.com/googlechrome/lighthouse/commit/7b0ac70))
* Fix eslint([afe3364](https://github.com/googlechrome/lighthouse/commit/afe3364))
* Update changelog script([11634e2](https://github.com/googlechrome/lighthouse/commit/11634e2))
* fix formatting for changelog([6b78e85](https://github.com/googlechrome/lighthouse/commit/6b78e85))
* web-inspector: fall back to page's Runtime and queryParam() ([#3497](https://github.com/googlechrome/lighthouse/pull/3497))
* Add pr links & remove issue references([8e26dfd](https://github.com/googlechrome/lighthouse/commit/8e26dfd))
* Create bug-labels.md([58930e1](https://github.com/googlechrome/lighthouse/commit/58930e1))
* Bump launcher to 0.8.1 ([#3479](https://github.com/googlechrome/lighthouse/pull/3479))
* removed unused audit meta.category ([#3554](https://github.com/googlechrome/lighthouse/pull/3554))
* Generate changelogs([623c4cc](https://github.com/googlechrome/lighthouse/commit/623c4cc))
* add CODEOWNERS file ([#3591](https://github.com/googlechrome/lighthouse/pull/3591))
* new-audit => new_audit ([#3534](https://github.com/googlechrome/lighthouse/pull/3534))
* Enable type checking for JavaScript ([#3589](https://github.com/googlechrome/lighthouse/pull/3589))
* provide svg logo as png([8b3d7f0](https://github.com/googlechrome/lighthouse/commit/8b3d7f0))
### Report
* reformat results, incl all requests and wasted time, ([#3492](https://github.com/googlechrome/lighthouse/pull/3492))
* improve actionability of helpText ([#3544](https://github.com/googlechrome/lighthouse/pull/3544))
### Tests
* update eslint (and goog config) to latest ([#3396](https://github.com/googlechrome/lighthouse/pull/3396))
* use --quiet flag rather than --silent ([#3491](https://github.com/googlechrome/lighthouse/pull/3491))
* disable multiple shadow root deprecation test ([#3695](https://github.com/googlechrome/lighthouse/pull/3695))
* Passive event listener violation doesn't report on passive:false now ([#3498](https://github.com/googlechrome/lighthouse/pull/3498))
* add test for setImmediate polyfill ([#3670](https://github.com/googlechrome/lighthouse/pull/3670)) |
hmm, still does |
groupBy: 'type', | ||
commitGroupsSort: (a, b) => { | ||
// put new audit on the top | ||
if (b.title === 'New audit') { |
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 isn't working for me. I think you also need to handle the other case, too (if (a.title === 'New audit') return -1
)
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.
my mistake! it works now!
commit.type = 'Misc'; | ||
} | ||
|
||
let pullRequestMatch = commit.header.match(/\(#(\d+)\)/); |
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.
what's the difference between this case and the message
one below? Is header
the initial line of the commit message?
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.
Also, should we be checking that the PR # is at the end of the string? (the default positioning from the squash and merge button). A #XXXX elsewhere could be e.g. a reference to an issue
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.
header is indeed the first line and message is the rest. When I only ran it on message I didn't get all the PR references if I check both I do.
For the regex, i checked the last 50 commits and we never refer an issue in the commit header but better save than sorry so edited the regex!
|
||
return a.title.localeCompare(b.title); | ||
}, | ||
commitsSort: ['type', 'scope', 'message'], |
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.
In the past we've only sorted by type
and scope
(if it exists) and then by commit order, but not sure if that's possible with this
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
--}} | ||
* {{#if message ~}} |
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.
include scope?
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.
💯 lgtm % nits
{{~@root.repository}}/{{@root.commit}}/{{hash}})) | ||
{{~/if~}} | ||
{{~else~}} | ||
{{~hash~}} |
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.
groupBy: 'type', | ||
commitGroupsSort: (a, b) => { | ||
// put new audit on the top | ||
if (a.title === 'New audit') { |
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 we similarly force Misc
to the bottom? feels a bit weird in the middle
I talked to @paulirish about @wardpeet feel free to revert commit or add changes on top of it :) |
the tagging thing is still messed up for me, but if it's working for you we can figure it out once it's on master |
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.
LGTM, over to @wardpeet to approve
@brendankenny thanks for the fixes. |
Generates a changelog when running
yarn changelog:generate