Skip to content

Support default custom output path - #1109

Closed
JonCubed wants to merge 16 commits into
angular:masterfrom
JonCubed:pr-817
Closed

JonCubed wants to merge 16 commits into
angular:masterfrom
JonCubed:pr-817

Conversation

@JonCubed

Copy link
Copy Markdown
Contributor

pull request for #817

  • updated test, init, new commands to support --output-path
  • added build and serve command
  • new, init, build, serve, test, deploy commands support default output path value from angular-cli.json
  • removed test and deploy commands hard-coded output path value

JonCubed added 8 commits June 15, 2016 19:45
allow outputPath as a default value in angular-cli.js for all commands to use. Allows for a consistent way to specify where commands should output artefacts or look for them.
add utility functions for commands
add output path flag to test command, allowing default to be overridden.
add build command to replace default build command to allow for output path default.
add serve command to replace default serve command to allow for output path default.
add output path flag to deploy command, allowing default to be overridden.
add output path flag to  init and new command, allowing a default output-path to be set on initialising a project
@JonCubed

Copy link
Copy Markdown
Contributor Author

@filipesilva @hansl

for build, test, serve commands unit testing them doesn't feel appropriate due to having to have a proper project initialised to test and will slow down the acceptance tests a lot I think. I can do it as e2e tests, would prefer me to add it to the existing basic workflow e2e or create new e2e tests?

}, {
name: 'output-path',
type: String,
default: 'dist/',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This has access to the config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added the flag for flexibility, there was a use case where someone wanted to have different environments build to folders, having a flag would allow this. If you still feel it is unnecessary I'm happy to back it out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Too many flags will be confusing to new users using ng --help. I say having it in the config is enough. I do think it's unnecessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok np I'll back it out

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

just another thought on this if we remove it there will not be a default value for outputPath if it's not set in the config file. I can handle this in CommandHelper.loadDefaults by making sure outputPath has a value.

Do you have any issues if I do it this way?

@googlebot

Copy link
Copy Markdown

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

@filipesilva

filipesilva commented Oct 9, 2016 •

Copy link
Copy Markdown
Contributor

Heya @JonCubed, we ended up implementing this in other PRs. I'm sorry we didn't take yours in, but thank you for all the work you put into it!

@filipesilva filipesilva closed this Oct 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants