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

chore(script): automatic example module creation #5350

Closed
wants to merge 79 commits into from

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Jun 24, 2017

Enables the creation of the examples module and metadata associated with it by using jsdoc decorator tags. There are 3 required tags:

  • title: the title to be shown
  • id: the selector for the examples. e.g. <!-- example(go-live) -->
  • component: the component class name

there are 3 other optional tags, these are:

  • addlComponents: Comma separated list of additional component class names required for the example. Required by snackbar component
  • additionalFiles: Comma separated list of additional files required for the example. Required by dialog component.
  • selectorName: Comma separated list of component class names required for the demo runner.

A simple example looks like:

/**
 * @title Stacked chips
 * @id chips-stacked
 * @component ChipsStackedExample
 */

more advanced scenarios such as snackbar and dialogs look like:

/**
 * @title Dialog elements
 * @id dialog-elements
 * @component DialogElementsExample
 * @addlComponents DialogElementsExampleDialog
 * @additionalFiles dialog-elements-example-dialog.html
 * @selectorName DialogElementsExample, DialogElementsExampleDialog
 */

Enables the creation of the examples module and metadata associated with it by using jsdoc decorator tags.
@amcdnl amcdnl requested a review from jelbourn June 24, 2017 19:41
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 24, 2017
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Initial batch of comment (fyi I tend to do multiple rounds of back-and-forth)

* @id dialog-elements
* @component DialogElementsExample
* @addlComponents DialogElementsExampleDialog
* @additionalFiles dialog-elements-example-dialog.html
Copy link
Member

Choose a reason for hiding this comment

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

Can we not detect additional files at the script level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, using something like a TypeScript parser we could detect these.

@@ -177,3 +180,197 @@ function createTagNameAliaser(classPrefix: string) {
return this;
};
}

interface ExampleMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

I'd put all the code related to generating the example module in its own file

Copy link
Contributor Author

@amcdnl amcdnl Jun 26, 2017

Choose a reason for hiding this comment

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

Ok, I started that at first but moved into the same file at the end because of similarities. Do you have a filename in mind?

@@ -4,6 +4,11 @@ import {FormControl} from '@angular/forms';
import 'rxjs/add/operator/startWith';
import 'rxjs/add/operator/map';

/**
* @title Basic autocomplete
* @id autocomplete-overview
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the directory name (minus the -example) part) as the ID for each example? One less thing to manually keep consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, sounds like a good idea. Just a hold over from previous code.

/**
* @title Basic buttons
* @id button-overview
* @component ButtonOverviewExample
Copy link
Member

Choose a reason for hiding this comment

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

Remind me why we can't extract the component name from the source?

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 first tried require(file) and looking at the exported names but caused runtime issues. I spoke w/ @devversion and we have a plan to do this in typescript parser.

* Builds the template for the examples module
* @param results
*/
const buildTemplate = (results: ExampleMetadata[]): string => {
Copy link
Member

Choose a reason for hiding this comment

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

For standalone functions like this, prefer function syntax:

function buildTemplate(...) {
}

.replace('.ts', '');

// get the decorator tags, not all required
const titleMatch = src.match(/@title.*/g);
Copy link
Member

Choose a reason for hiding this comment

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

Would this be easier to do this with the TypeScript compiler API rather than regexes?

Copy link
Member

@devversion devversion Jun 25, 2017

Choose a reason for hiding this comment

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

Yeah most of the additionalFiles and additionalComponents could be detected using the TypeScript AST.

The only thing that is problematic would be the "demo" title for a given demo (because it wouldn't fit into the metadata)

Although the title could be part of the JSDoc of a component.

/**
 * @title Test Demo
 * @id my-id
 */
@Component({
  ...
}
export class MyTestExample {}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the title (and probably a description and maybe some other stuff later) will definitely have to be comments, but I think we can infer everything else.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I think the TypeScript API also provides an easy way to retrieve the JSDoc of a given Node.

*/
const buildTemplate = (results: ExampleMetadata[]): string => {
// create the import statements
const imports = (res: ExampleMetadata): string => {
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer more meaningful variable names without shortening (e.g, extractedMetadata instead of result)

};

// create the examples metadat
const examples = (res: ExampleMetadata): string => {
Copy link
Member

Choose a reason for hiding this comment

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

Generally prefer not nesting functions like this rather than having them as standalone siblings. It makes it hard to separate the individual function behavior from the containing function.

};

return `
import {NgModule} from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to move this template to its own file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn - Where would you like this file to live?

Copy link
Member

Choose a reason for hiding this comment

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

Actually let's see how small it gets when moving the material modules into their own file; maybe then it will be short enough to leave as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, it ended up pretty small after that.

MdListModule, MdMenuModule, MdProgressBarModule, MdProgressSpinnerModule,
MdRadioModule, MdSelectModule, MdSidenavModule, MdSliderModule,
MdSlideToggleModule, MdSnackBarModule, MdTabsModule, MdToolbarModule, MdTooltipModule
} from '@angular/material';
Copy link
Member

Choose a reason for hiding this comment

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

ExampleMaterialModule and LiveExample can go in their own TS files and just be imported rather than included in the template.

@jelbourn jelbourn requested a review from devversion June 25, 2017 16:06
@jelbourn
Copy link
Member

@devversion should also take a look

@amcdnl
Copy link
Contributor Author

amcdnl commented Jun 26, 2017

Per feedback from @devversion -

  • Resolve every export of a ts.SourceFile. There is a getExportsOfModule function
  • Walk through exports and check if it's a Component and if so -> analyze the decorator function call (resolve templateUrl and styleUrls) (edited)
  • Resolve example title and example id from JSDoc (using getDocumentationComment())

@amcdnl
Copy link
Contributor Author

amcdnl commented Jun 28, 2017

@devversion @jelbourn - I believe I have addressed the outstanding items. Can you please re-review?

* Build the import template
*
* @param {ExampleMetadata} metadata
* @returns {string} template
Copy link
Member

Choose a reason for hiding this comment

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

Omit type from all JsDoc since it's captured by TypeScript (things like @param and @return can still have a description, though)

}

/**
* Build the import template
Copy link
Member

Choose a reason for hiding this comment

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

Elaborate on what "the import template" is?

* @returns {string} template
*/
function buildExamplesTemplate(metadata: ExampleMetadata): string {
const addlFiles = metadata.additionalFiles ?
Copy link
Member

Choose a reason for hiding this comment

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

additionalFiles
(generally prefer to avoid abbreviations)

*/
function buildExamplesTemplate(metadata: ExampleMetadata): string {
const addlFiles = metadata.additionalFiles ?
JSON.stringify(metadata.additionalFiles) : 'undefined';
Copy link
Member

Choose a reason for hiding this comment

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

Why use a string 'undefined' instead of, say, null?
(add a comment?)

JSON.stringify(metadata.additionalFiles) : 'undefined';

const selectorName = metadata.selectorName ?
("'" + metadata.selectorName.join(', ') + "'") : 'undefined';
Copy link
Member

Choose a reason for hiding this comment

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

Template string?

`'${metadata.selectorName.join(', ')}'`

* @param {string} src
* @returns {{ primary: any, seconds: any[] }}
*/
function createMetas(filename: string, src: string): { primaryComponent: any, secondaryComponents: any[] } {
Copy link
Member

Choose a reason for hiding this comment

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

Can we tighten up the any use in this function? Or does the ts parser ironically not offer specific types?

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 added a new interface for the parsed metadata.

I was having type issues with the visit function node, I'll revisit it and see if I can't tighten it 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.

@devversion - I tinkered with tightening up the types in the visit function but had no luck.

I started off w/ const visit = (node: ts.Node) then when I hit ClassDeclaration I casted to that type, however, jsDoc is not a type on that node. I looked up the types and jsDoc is actually not a type on anything that extends ts.Node (odd?).

Also when I casted it, the decorator.expression.expression.text was no longer accessible. I tried the methods called getText() like we talked about but all returned TypeError: Cannot read property 'text' of undefined.

/**
* Creates the examples module and metadata
*/
task('build-examples-module', (cb) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is the callback necessary if the task is synchronous? If so I'd call it done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, you need to wait until all operations are completed before we mark the task completed.

}

/**
* Build the list template
Copy link
Member

Choose a reason for hiding this comment

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

Elaborate on what "the list template" is?

* @param {ExampleMetadata[]} extractedMetadata
* @returns {string} resulting template
*/
function buildTemplate(extractedMetadata: ExampleMetadata[]): string {
Copy link
Member

Choose a reason for hiding this comment

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

Is this just generateExampleNgModule?

}

/**
* Build the examples template
Copy link
Member

Choose a reason for hiding this comment

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

Elaborate on what "the examples template" is?


const { primaryComponent, secondaryComponents } = createMetas(match, src);

if(primaryComponent) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Space after the if here.

if(primaryComponent) {
// convert the class name to dashcase id
let id = primaryComponent.component.replace('Example', '');
id = convertToDashCase(id);
Copy link
Member

Choose a reason for hiding this comment

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

You can make this is a const if you just combine the variable initialization.

convertToDashCase(primaryComponent.component.replace('Example', ''));

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 try to avoid over complicated function invocation, but happy to change.

selectorName: []
};

if(secondaryComponents.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Space missing here and in the for loop below as well. (Do we have a lint rule for this?)

task('build-examples-module', (cb) => {
const results: ExampleMetadata[] = [];

glob('src/material-examples/**/*.ts', (err, matches) => {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid the asynchronous here, just import glob.sync (as we do everywhere else)

import {sync as glob} from 'glob';

@amcdnl
Copy link
Contributor Author

amcdnl commented Jun 28, 2017

@jelbourn @devversion - Updated and ready for further review.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Just a last couple of style nits

for(const tag of doc.tags) {
const tagValue = tag.comment;
const tagName = tag.tagName.text;
if(tagName === 'title') {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: space between keyword and parenthesis (if, for)
(here and elsewhere)

* Parse the AST of a file and get metadata about it
*/
function parseExampleMetadata(filename: string, src: string):
{ primaryComponent: ParsedMetadata, secondaryComponents: ParsedMetadata[] } {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: no leading/trailing space in object literals
(here and elsewhere)


interface ExampleMetadata {
component: string;
filename: string;
Copy link
Member

Choose a reason for hiding this comment

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

fileName as for consistency with TypeScript API

components.push(...metadata.additionalComponents);
}

return `import {${components.join(',')}} from '${metadata.filename}';
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment from where this template imports. This should explain what the root directory of this file is.

Also since the fileName (as in another comment) should be only the name, you should add the ./ prefix just here.

const src = fs.readFileSync(file, 'utf-8');
const filename = file
.replace('src/material-examples/', './')
.replace('.ts', '');
Copy link
Member

Choose a reason for hiding this comment

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

Related to the other comments. This can be just path.basename(filePath); (the .ts extension should be fine to stay)

@amcdnl
Copy link
Contributor Author

amcdnl commented Jun 30, 2017

@devversion @jelbourn Ready for another review :)

@jelbourn
Copy link
Member

tslint is now complaining about the generated file. Should add /* tslint:disable */ at the top of the generated file to avoid this.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Final round of comments from me.

import {buildConfig} from 'material2-build-tools';
import * as fs from 'fs';
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add the fs import here?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, there is no readFileSync and writeFileSync in the docs.ts file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was left over from my code in that file. I will remove.


/**
* Build ecmascript module import statements
*/
Copy link
Member

Choose a reason for hiding this comment

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

This can be a a single-line comment.

/** Build ES2015 module imports for the specified example metadata */
function buildImportsTemplate(metadata: ExampleMetadata): string {

* Build ecmascript module import statements
*/
function buildImportsTemplate(metadata: ExampleMetadata): string {
const components = [metadata.component];
Copy link
Member

Choose a reason for hiding this comment

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

const components =[metadata.component, ...metadata.additonalComponents];

You don't really need the check if there are additionalComponents. The additionalComponents property is always an array.

Alternative Solution:

const components = metadata.additonalComponents.concat(metadata.component)

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, left over from when it wasn't.

components.push(...metadata.additionalComponents);
}

// imports the template from the /src/material-examples directory
Copy link
Member

Choose a reason for hiding this comment

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

// Create a relative path to the source file of the current example.
// The relative path will be used inside of a TypeScript import statement.

// imports the template from the /src/material-examples directory
const relativeSrcPath = path
.relative('./src/material-examples', metadata.sourcePath)
.replace('.ts', '');
Copy link
Member

Choose a reason for hiding this comment

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

We also need to replace the Windows backslashes here.


if (metadata.additionalComponents) {
components.push(...metadata.additionalComponents);
}
Copy link
Member

Choose a reason for hiding this comment

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

This would be the same comment as for the imports. We should be able to drop the if check here.

const { primaryComponent, secondaryComponents } = parseExampleMetadata(sourcePath, src);

if (primaryComponent) {
// convert the class name to dashcase id
Copy link
Member

Choose a reason for hiding this comment

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

// Generate a unique id for the component by converting the class name to dash-case.

};

if (secondaryComponents.length) {
// for whatever reason the primary is listed here
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't really help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More of a reminder for myself, I'll remove.

}
}

const template = generateExampleNgModule(results);
Copy link
Member

Choose a reason for hiding this comment

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

const generatedModuleFile = generateExampleNgModule(results);

/**
* Creates the examples module and metadata
*/
task('build-examples-module', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Since we are now using gulp.sync we can just get rid of the done here.

@amcdnl
Copy link
Contributor Author

amcdnl commented Jul 1, 2017

@devversion @jelbourn - Ready for another review

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. Three minor comments and then I think it's good to go.


const matchedFiles = glob(path.join(examplesPath, '**/*.ts'));
for (const sourcePath of matchedFiles) {
const src = fs.readFileSync(sourcePath, 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

The variable name src is pretty unclear. How about fileContent/sourceContent?

component: node.name.text
};

let primary = false;
Copy link
Member

Choose a reason for hiding this comment

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

This variable seems to be unused?

*/
function buildExamplesTemplate(metadata: ExampleMetadata): string {
// if no additional files or selectors were provided,
// return undefined since we don't care about if these were not found
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change this comment to null now as well (since it's no longer undefined now). Maybe something like

// If there are no additional files or selectors specified, just set those variables
// to an empty array.
const additionalFiles = metadata.additionalFiles.length ? 
   JSON.stringify(metadata.additionalFiles) : '[]';

Not sure if an empty array would make sense here. It would allow us to drop more null checks in the docs app. But null is also fine.

amcdnl and others added 2 commits July 8, 2017 10:50
Adds a Stylelint rule that will prevent uses of nested mixins. This will help prevent issues like angular#5232 in the future.
devversion and others added 22 commits July 8, 2017 11:29
* Randomizes the order of test execution inside of Jasmine / Karma.
…ngular#5547)

* Most of the `whenStable()` promise calls can be replaced with the `flushMicrotasks` method, which just flushes them instead of waiting for them to really happen (when using `async`).
* This reduces nesting of functions and also makes the tests in theory faster.
* With angular#5429 dgeni runs twice when running the `gulp docs` command. This is because the dgeni generate function has been added to the `dgeni/index.js` file.
* e2e(input): add textarea e2e test

* test(input): remove fdescribe
The correct link is `/guide/cdk-table` instead of ~/guides/cdk-
…ar#5347)

Uses the menu panel's overlay position to determine the animation origin, instead of the trigger. These changes make it easier to follow and avoid having to correct it explicitly based on the `overlapTrigger` flag, in addition to fixing some issues in nested menus.
Switches the `md-list` and `md-list-item` to `OnPush` change detection.

Relates to angular#5035.
Fixes an issue that caused the wrong amount of options to be read out by NVDA. E.g. if the select has 3 options, NVDA reads out "<value> selected, 2 of 4". The issue seems to come from the fact that NVDA considers the trigger as one of the options, potentially because it's clickable.
…gular#5333)

Prevents `textarea` instances from resizing beyond their `md-input-container`.
Fixes `md-chip` instances not being visible for users running high contrast mode.
…ular#5328)

Fixes being able to set a disabled `md-select` to `touched` by clicking on it.
Adds an option to the dialog config that allows for the `aria-describedby` to be set on the dialog container.
…lar#5144)

* fix(tooltip): remove native event listener on component destroy

Component should clean up events it has registered to.

Closes angular#4499

* Break at higher syntactic level

* Undo accidental formatting
* Switches the snack bar component to `OnPush` change detection.
* Removes the `detectChanges` workaround for the choppy animations, because it no longer works with `OnPush`. Properly fixes the initial issue by providing a `void` animation state.

Relates to angular#5035.
…ngular#5555)

* includes @angular/cdk in github material installation instructions

* Mention the latest builds instructions

Add the latest builds instructions in the installation section (Step 1).

* Delete the installation section

The Getting Started section has all the information necessary to proceed with installations: angular#5552 (comment)
* Now TSLint starts reporting unused variables again (Rule has been un-deprecated and now uses the type checker
* In favor of linting the whole project the tsconfig file inside of `src/` needs to live in the project root.
* The root tsconfig file used to have proper autocompletion in editors like WebStorm is currently not working correctly. The path to the `@angular/cdk/testing` package is mapped to a non-existing folder.
* The PromiseCompleter is not used anywhere and in reality everbody can implement such a thing on his own. Also since there is the best practice to resolve/reject the promise while being inside of the anonymous function.
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 8, 2017
@amcdnl
Copy link
Contributor Author

amcdnl commented Jul 8, 2017

I'm closing this PR and gonna redo it given all the merge issues.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla
Projects
None yet
Development

Successfully merging this pull request may close these issues.