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

feat(up): hlx up defaults to local repo #1149

Merged
merged 6 commits into from
Sep 12, 2019
Merged

feat(up): hlx up defaults to local repo #1149

merged 6 commits into from
Sep 12, 2019

Conversation

stefan-guggisberg
Copy link
Contributor

fix #913

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #1149 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1149   +/-   ##
=======================================
  Coverage   92.02%   92.02%           
=======================================
  Files          44       44           
  Lines        1830     1830           
=======================================
  Hits         1684     1684           
  Misses        146      146
Impacted Files Coverage Δ
src/up.js 85% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 387ce6e...6afa114. Read the comment docs.

trieloff
trieloff previously approved these changes Sep 10, 2019
Copy link
Contributor

@tripodsan tripodsan left a comment

Choose a reason for hiding this comment

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

please add test for --no-local-repo

@tripodsan
Copy link
Contributor

tripodsan commented Sep 11, 2019

@stefan-guggisberg I added a commit that adds more tests and removes the --no-local-repo option. this is not required since yargs does this automatically for --no-* args.
feel free to change it again.

stefan-guggisberg and others added 2 commits September 11, 2019 09:25
also remove --no-local-repo since this is automatically handled by yargs.
@stefan-guggisberg
Copy link
Contributor Author

stefan-guggisberg commented Sep 11, 2019

please add test for --no-local-repo

there already was a test for --no-local-repo:

helix-cli/test/testUpCli.js

Lines 212 to 218 in 387ce6e

it('hlx up can specify no local repo', () => {
new CLI()
.withCommandExecutor('up', mockUp)
.run(['up', '--no-local-repo']);
sinon.assert.calledWith(mockUp.withLocalRepo, []);
sinon.assert.calledOnce(mockUp.run);
});

@stefan-guggisberg
Copy link
Contributor Author

@stefan-guggisberg I added a commit that adds more tests and removes the --no-local-repo option. this is not required since yargs does this automatically for --no-* args.

@tripodsan I learned about the automagic support for --no-* args the hard way, i.e. I wasted too much time debugging yargs. It's not well documented :/

I explicitly added the --no-local-repo option in order to show up on hlx up --help.

@tripodsan
Copy link
Contributor

I explicitly added the --no-local-repo option in order to show up on hlx up --help.

but then we should also add it to the help for hlx up --no-open, just to be consistent.

@stefan-guggisberg
Copy link
Contributor Author

regarding the failing smoke test:
adobe/project-helix.io#62

@tripodsan tripodsan merged commit 62affcb into master Sep 12, 2019
@tripodsan tripodsan deleted the issue-913 branch September 12, 2019 00:03
trieloff pushed a commit that referenced this pull request Sep 12, 2019
# [5.2.0](v5.1.0...v5.2.0) (2019-09-12)

### Features

* **up:** hlx up defaults to local repo ([#1149](#1149)) ([62affcb](62affcb))
@adobe-bot
Copy link
Collaborator

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--local-repo=. should be default
4 participants