fix(@angular/cli): fix issue of folder getting generated on dry run #6026
Conversation
|
I think the underlying issue is that the dynamic path parser is doing more than just parsing. The directory creation probably shouldn't be there. |
|
@clydin Could be but then we would have to replicate it for all. Currently its centralized at one place and any change we need to make in regards of the paths we can do it here I believe. |
There was a problem hiding this comment.
Thanks for that @sumitarora.
I'm not sure if these strings: sda and asda should be here. So I pointed that out.
| @@ -506,10 +506,10 @@ Blueprint.prototype.install = function(options) { | |||
| ui.writeLine('installing ' + this.name); | |||
|
|
|||
| if (dryRun) { | |||
| ui.writeLine(chalk.yellow('You specified the dry-run flag, so no' + | |||
| ui.writeLine(chalk.yellow('You specified the dry-run sda flag, so no' + | |||
There was a problem hiding this comment.
Sorry if I'm wrong, but it seems weird the sda. Is that a typo?
'You specified the dry-run
sdaflag, so no'
| @@ -541,7 +541,7 @@ Blueprint.prototype.uninstall = function(options) { | |||
| ui.writeLine('uninstalling ' + this.name); | |||
|
|
|||
| if (dryRun) { | |||
| ui.writeLine(chalk.yellow('You specified the dry-run flag, so no' + | |||
| ui.writeLine(chalk.yellow('You specified asda the dry-run flag, so no' + | |||
There was a problem hiding this comment.
(again) Sorry if I'm wrong, but it seems weird the asda. Is that a typo?
'You specified
asdathe dry-run flag, so no'
sumitarora
force-pushed
the
fix-dry-run-folder
branch
from
f6df0f8 to
2dfeaae
Compare
|
@leocaseiro Thanks for review updated typos :P |
| @@ -3,7 +3,8 @@ import * as process from 'process'; | |||
| import * as fs from 'fs'; | |||
| const stringUtils = require('ember-cli-string-utils'); | |||
|
|
|||
| export function dynamicPathParser(project: any, entityName: string, appConfig: any) { | |||
| export function dynamicPathParser | |||
There was a problem hiding this comment.
Parentheses should be on the same line. This should probably look like:
export function dynamicPathParser(project: any, entityName: string, appConfig: any,
dryRun: boolean) {
|
The new Schematics will fix those errors entirely by not allowing side effects in blueprints. So I'm fine with patching dynamicPathParser for now. |
sumitarora
force-pushed
the
fix-dry-run-folder
branch
from
2dfeaae to
fb26b15
Compare
| @@ -3,7 +3,8 @@ import * as process from 'process'; | |||
| import * as fs from 'fs'; | |||
| const stringUtils = require('ember-cli-string-utils'); | |||
|
|
|||
| export function dynamicPathParser(project: any, entityName: string, appConfig: any) { | |||
| export function dynamicPathParser(project: any, entityName: string, appConfig: any, | |||
| dryRun: boolean) { | |||
There was a problem hiding this comment.
This should be defaulted to false instead of having to pass it in every call.
There was a problem hiding this comment.
@Brocco what do you think of passing in options instead?
There was a problem hiding this comment.
I think the options change would be a nice one, for when additional things are added. It will also simplify the defaulting of values assuming a class is used instead of an interface.
sumitarora
force-pushed
the
fix-dry-run-folder
branch
from
7aaa396 to
b90d56e
Compare
| appConfig: any; | ||
| dryRun: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
To allow defaults and improve encapsulation, what about something like:
export class DynamicPathOptions {
projectRoot: string;
sourceDirectory: string;
entityName: string;
dryRun?: boolean = true;
}
And then something like this in the function:
options = { ...new DynamicPathOptions, ...options };
b90d56e to
2294dd5
Compare
|
@hansl are you happy with the changes? |
sumitarora
force-pushed
the
fix-dry-run-folder
branch
from
2294dd5 to
672b5f5
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes: #6017