Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

Ng schematics pipe #20

Merged
merged 1 commit into from
Jul 13, 2017
Merged

Ng schematics pipe #20

merged 1 commit into from
Jul 13, 2017

Conversation

jschwarty
Copy link
Contributor

The module option still needs to be implemented. I didn't see an example of that yet in the component schematic so I am not sure how the desired implementation should go (looks like it will probably need a resolveModulePath utility function).

@Brocco can you check this out and let me know if I got everything (besides the module option) please? 😄

Copy link

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Just minor nits. Looking good so far!

It's nice that you included the help text for all of the commands, even when the component schema does not.

* found in the LICENSE file at https://angular.io/license
*/
import { addDeclarationToModule, addExportToModule } from '../utility/ast-utils';
import {InsertChange} from '../utility/change';
Copy link

Choose a reason for hiding this comment

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

Inconsistent spacing within brackets :)

noop,
normalizePath,
template,
url,
Copy link

Choose a reason for hiding this comment

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

Comma not needed?


function addDeclarationToNgModule(options: any): Rule {
return (host: Tree) => {
if (options['skip-import']) {
Copy link

Choose a reason for hiding this comment

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

This should probably be consistent with https://github.com/angular/devkit/blob/master/packages/schematics/angular/component/index.ts#L33? I'm not sure which of the two should be preferred.

"default": true,
"description": "Specifies if a spec file is generated."
},
"skip-import": {
Copy link

@Splaktar Splaktar Jul 5, 2017

Choose a reason for hiding this comment

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

@jschwarty
Copy link
Contributor Author

Thanks for the initial feedback @Splaktar! Got those code format changes made and changed skip-import to camel case.

@Brocco
Copy link
Contributor

Brocco commented Jul 5, 2017

This looks good, sans the module import code which I'm refactoring for the angular-app schematic. I should have that pushed in a few hours which will allow you to update this PR.

@Brocco
Copy link
Contributor

Brocco commented Jul 12, 2017

@jschwarty PR #29 contains the updates for module resolutions.

@jschwarty jschwarty force-pushed the ng-schematics-pipe branch from aa06a81 to ee48844 Compare July 13, 2017 20:00
@Brocco Brocco merged commit 18fe75d into angular:master Jul 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants