Skip to content

Conversation

@JonCubed
Copy link
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
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
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
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
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
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
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

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

CLAs look good, thanks!

@filipesilva
Copy link
Contributor

filipesilva commented Oct 9, 2016

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
@JonCubed
Copy link
Contributor Author

@filipesilva no worries

@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 10, 2019
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