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

Clean shell scripts #7826

Merged
merged 1 commit into from Mar 22, 2018
Merged

Clean shell scripts #7826

merged 1 commit into from Mar 22, 2018

Conversation

@zapashcanon
Copy link
Contributor

zapashcanon commented Mar 13, 2018

Hi,

I fixed various things spotted by shellcheck.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

Copy link

elamperti left a comment

Good work here, @zapashcanon ! 👍

npm publish
cd -
)

This comment has been minimized.

Copy link
@elamperti

elamperti Mar 16, 2018

This is a nice trick! Though I'm not sure if it'd be clear for everyone what is it doing. I'd stick with the cd - just for clarity.

This comment has been minimized.

Copy link
@zapashcanon

zapashcanon Mar 18, 2018

Author Contributor

I can add a comment like: using a subshell to avoid having to cd back.

It's not just to avoid writing a line, it also can prevent some troubles, see SC2013.

This comment has been minimized.

Copy link
@elamperti

elamperti Mar 18, 2018

Oh, I didn't know about that one! It should be modified then. The comment would be helpful :)

This comment has been minimized.

Copy link
@zapashcanon

zapashcanon Mar 18, 2018

Author Contributor

Done.

if [[ -z $RELEASE_TAG ]]; then
npm publish
else
npm publish --tag $RELEASE_TAG
npm publish --tag "$RELEASE_TAG"
fi
cd -

This comment has been minimized.

Copy link
@elamperti

elamperti Mar 16, 2018

In any case, for consistency, shouldn't this cd - be replaced as well?

This comment has been minimized.

Copy link
@zapashcanon

zapashcanon Mar 18, 2018

Author Contributor

Indeed. Will do. I'm not sure if shellcheck would have spotted it after running it a second time. Will try. If not, we may found a bug....

This comment has been minimized.

Copy link
@zapashcanon

zapashcanon Mar 18, 2018

Author Contributor

Opened an issue. :)

@yyx990803 yyx990803 merged commit 943e5c2 into vuejs:dev Mar 22, 2018
5 checks passed
5 checks passed
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint-flow-types Your tests passed on CircleCI!
Details
ci/circleci: test-cover Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-ssr-weex Your tests passed on CircleCI!
Details
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.