Skip to content
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

Commit PR notes to draftlogs and add scripts to provide draft release notes (CHANGELOG.md) and empty draftlogs #5780

Merged
merged 10 commits into from
Jul 7, 2021
17 changes: 10 additions & 7 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ Thanks for your interest in plotly.js!
### Features, Bug fixes, and others:

Before opening a pull request, developer should:
1. make sure they are not on the `master` branch of their fork as using `master` for a pull request would make it difficult to fetch `upstream` changes.
2. fetch latest changes from `upstream/master` into your fork i.e. `origin/master` then pull `origin/master` from you local `master`.
3. then `git rebase master` their local dev branch off the latest `master` which should be sync with `upstream/master` at this time.
4. make sure to **not** `git add` the `dist/` folder (the `dist/` is updated only on version bumps).
5. make sure to commit changes to the `package-lock.json` file (if any new dependency required).
6. provide a title and write an overview of what the PR attempts to do with a link to the issue they are trying to address.
7. select the _Allow edits from maintainers_ option (see this [article](https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/) for more details).

- `git rebase` their local branch off the latest `master`,
- make sure to **not** `git add` the `dist/` folder (the `dist/` is updated only on version bumps),
- make sure to commit changes to the `package-lock.json` file (if any new dependency required),
- write an overview of what the PR attempts to do,
- select the _Allow edits from maintainers_ option (see this [article](https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/) for more details).

Note that it is forbidden to force push (i.e. `git push -f`) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please `git merge master` into your PR branch instead of `git rebase master`.
After opening a pull request, developer:
- should create a new small markdown log file using the PR number e.g. `1010_fix.md` or `1010_add.md` inside `draftlogs` folder as described in this [README](https://github.com/plotly/plotly.js/blob/master/draftlogs/README.md), commit it and push.
- should **not** force push (i.e. `git push -f`) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please fetch `upstream/master` and "merge" with master instead of "rebase".
2 changes: 2 additions & 0 deletions draftlogs/5779_fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Improve README for ES6 module import [[#5779](https://github.com/plotly/plotly.js/pull/5779)],
with thanks to @andreafonso for the contribution!
22 changes: 22 additions & 0 deletions draftlogs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
## Directory of draft logs to help prepare the upcoming [CHANGELOG](https://github.com/plotly/plotly.js/blob/master/CHANGELOG.md)

It is required that the PR contributors start the filename with a number.
This number should preferably be the PR number.
The number of issue they are trying to close could also be used.
The markdown file should end with one of the followings:
1. `_fix.md` to propose a bug fix
2. `_add.md` to propose new features
3. `_remove.md` to propose a feature removal
4. `_change.md` to propose a minor/major change
5. `_deprecate.md` to propose a feature deprecate

### Example filename and content for PR numbered 5546 for adding a new feature
- filename: `5546_add.md`
- content:
```
- Add `icicle` trace type [[#5546](https://github.com/plotly/plotly.js/pull/5546)]
```
which would render
- Add `icicle` trace type [[#5546](https://github.com/plotly/plotly.js/pull/5546)]

> Please start your single-line or multiple lined message with a verb. You could basically use the PR description while providing a link to the PR similar to the above example is appreciated too.
7 changes: 5 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@
"stats": "node tasks/stats.js",
"find-strings": "node tasks/find_locale_strings.js",
"preprocess": "node tasks/preprocess.js",
"build": "node tasks/empty_dist.js && npm run preprocess && npm run find-strings && npm run bundle && npm run extra-bundles && npm run stats",
"cibuild": "node tasks/empty_dist.js && npm run preprocess && node tasks/cibundle.js",
"use-draftlogs": "node tasks/use_draftlogs.js",
"empty-draftlogs": "node tasks/empty_draftlogs.js",
"empty-dist": "node tasks/empty_dist.js",
"build": "npm run empty-dist && npm run preprocess && npm run find-strings && npm run bundle && npm run extra-bundles && npm run stats",
"cibuild": "npm run empty-dist && npm run preprocess && node tasks/cibundle.js",
"watch": "node tasks/watch.js",
"lint": "eslint --version && eslint .",
"lint-fix": "eslint . --fix || true",
Expand Down
31 changes: 4 additions & 27 deletions tasks/empty_dist.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,13 @@
var path = require('path');
var fs = require('fs-extra');
var common = require('./util/common');
var constants = require('./util/constants');
var makeEmptyDirectory = require('./util/make_empty_directory');
var emptyDir = makeEmptyDirectory.emptyDir;
var makeDir = makeEmptyDirectory.makeDir;

var dist = constants.pathToDist; // dist
var distTopojson = constants.pathToTopojsonDist; // dist/topojson

// main
emptyDir(distTopojson);
emptyDir(dist);
makeDir(dist);
makeDir(distTopojson);

function emptyDir(dir) {
if(common.doesDirExist(dir)) {
console.log('empty ' + dir);
try {
var allFiles = fs.readdirSync(dir);
allFiles.forEach(function(file) {
// remove file
fs.unlinkSync(path.join(dir, file));
});

fs.rmdirSync(dir);
} catch(err) {
console.error(err);
}
}
}

function makeDir(dir) {
if(!common.doesDirExist(dir)) {
// create folder
fs.mkdirSync(dir);
}
}
18 changes: 18 additions & 0 deletions tasks/empty_draftlogs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
var constants = require('./util/constants');
var makeEmptyDirectory = require('./util/make_empty_directory');
var emptyDir = makeEmptyDirectory.emptyDir;
var makeDir = makeEmptyDirectory.makeDir;

var pathToDraftlogs = constants.pathToDraftlogs;

var chZero = '0'.charCodeAt(0);
var chNine = '9'.charCodeAt(0);

function startsWithNumber(v) {
var ch = v.charCodeAt(0);
return chZero <= ch && ch <= chNine;
}

// main
emptyDir(pathToDraftlogs, { filter: startsWithNumber });
makeDir(pathToDraftlogs);
86 changes: 86 additions & 0 deletions tasks/use_draftlogs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
var fs = require('fs');
var path = require('path');
var constants = require('./util/constants');

var pathToDraftlogs = constants.pathToDraftlogs;
var pathToChangelog = constants.pathToChangelog;

var chZero = '0'.charCodeAt(0);
var chNine = '9'.charCodeAt(0);

function startsWithNumber(v) {
var ch = v.charCodeAt(0);
return chZero <= ch && ch <= chNine;
}

var allLogs = fs.readdirSync(pathToDraftlogs).filter(startsWithNumber);

var len = allLogs.length;
if(!len) return;

var writeAfterMe = 'where X.Y.Z is the semver of most recent plotly.js release.';
var changelog = fs.readFileSync(pathToChangelog).toString().split(writeAfterMe);
var head = changelog[0];
var foot = changelog[1];

var all = {
Added: [],
Removed: [],
Deprecated: [],
Changed: [],
Fixed: []
};

var ENTER = '\n';

var skippedFiles = [];
for(var i = 0; i < len; i++) {
var filename = allLogs[i];
var message = fs.readFileSync(path.join(pathToDraftlogs, filename), { encoding: 'utf-8' }).toString();
// trim empty lines
message = message.split(ENTER).filter(function(e) { return !!e; }).join(ENTER);

if(filename.endsWith('_add.md')) {
all.Added.push(message);
} else if(filename.endsWith('_remove.md')) {
all.Removed.push(message);
} else if(filename.endsWith('_deprecate.md')) {
all.Deprecated.push(message);
} else if(filename.endsWith('_change.md')) {
all.Changed.push(message);
} else if(filename.endsWith('_fix.md')) {
all.Fixed.push(message);
} else {
skippedFiles.push(filename);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Else throw an error, logging the filename as having been ignored.

Might want to also loosen the endsWith tests - to eg filename.toLowerCase().indexOf('_add') !== -1 so stuff like 5678_Added.md will still be picked up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a test which disallows use of uppercase in filenames.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do test for uppercase, but I don't think it'll dig into the draftlogs dir. And there's still add vs adds vs added etc. Anyway the main thing is to throw an error if we ignore the file. After adding that if you still want to be strict about names that's ok, it'll just occasionally make more work during release if we get an incorrect name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Addressed in 3a91c0e.

}

var draftNewChangelog = [
head + writeAfterMe,
'',
'## [X.Y.Z] -- UNRELEASED'
];

var append = function(key) {
var newMessages = all[key];
if(!newMessages.length) return;
draftNewChangelog.push('');
draftNewChangelog.push('### ' + key);
draftNewChangelog.push(newMessages.join(ENTER));
};

append('Added');
append('Removed');
append('Deprecated');
append('Changed');
append('Fixed');

draftNewChangelog.push(foot);

fs.writeFileSync(pathToChangelog, draftNewChangelog.join(ENTER), { encoding: 'utf-8' });

if(skippedFiles.length) {
throw JSON.stringify({
'skippedFiles': skippedFiles
}, null, 2);
}
3 changes: 3 additions & 0 deletions tasks/util/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ var pathToSrc = path.join(pathToRoot, 'src/');
var pathToLib = path.join(pathToRoot, 'lib/');
var pathToImageTest = path.join(pathToRoot, 'test/image');
var pathToStrictD3Module = path.join(pathToRoot, 'test/strict-d3.js');
var pathToDraftlogs = path.join(pathToRoot, 'draftlogs/');
var pathToDist = path.join(pathToRoot, 'dist/');
var pathToBuild = path.join(pathToRoot, 'build/');

Expand Down Expand Up @@ -168,6 +169,8 @@ module.exports = {
pathToLib: pathToLib,
pathToBuild: pathToBuild,
pathToDist: pathToDist,
pathToDraftlogs: pathToDraftlogs,
pathToChangelog: path.join(pathToRoot, 'CHANGELOG.md'),

partialBundleTraces: partialBundleTraces,

Expand Down
40 changes: 40 additions & 0 deletions tasks/util/make_empty_directory.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
var path = require('path');
var fs = require('fs-extra');
var common = require('./common');

module.exports = {
emptyDir: emptyDir,
makeDir: makeDir
};

function emptyDir(dir, opts) {
if(common.doesDirExist(dir)) {
console.log('empty ' + dir);
try {
var allFiles = fs.readdirSync(dir);
var hasFilter = false;
if(opts && opts.filter) {
hasFilter = true;
allFiles = allFiles.filter(opts.filter);
}

allFiles.forEach(function(file) {
// remove file
fs.unlinkSync(path.join(dir, file));
});

if(!hasFilter) {
fs.rmdirSync(dir);
}
} catch(err) {
console.error(err);
}
}
}

function makeDir(dir) {
if(!common.doesDirExist(dir)) {
// create folder
fs.mkdirSync(dir);
}
}