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

Add missing space causing parse error. #4002

Merged
merged 1 commit into from Jun 15, 2015

Conversation

aliafshar
Copy link
Contributor

No description provided.

@aliafshar
Copy link
Contributor Author

Was getting an error on install and on starting zsh.

@ncanceill
Copy link
Contributor

@robbyrussell there was a typo in #3986, please merge this ASAP to fix

@oryband
Copy link

oryband commented Jun 15, 2015

yup, kinda crashes zsh now
please merge!

@fredr
Copy link

fredr commented Jun 15, 2015

👍

@mcornella
Copy link
Member

Please add fixes #4009, fixes #4010, fixes #4011 to your PR description so those duplicates are automatically closed when this PR is merged.

@kevin-lee
Copy link

This closes #4018, #4019, #4020, #4021 and #4022 as well.

@leepa
Copy link

leepa commented Jun 15, 2015

Can we get this merged asap?

@webmaster128
Copy link

shellcheck would have found that one. Has someone an idea how to automatically shellcheck PRs in the future?

@kevin-lee
Copy link

Probably using some CI service might be helpful (e.g. Semaphore, Travis, Codeship, etc.)? I'm not sure if there's any CI service supporting shellcheck though.

@kevin-lee
Copy link

@aliafshar For your convenience, you can simply add the following line to your PR description in order to close the following tickets when this PR is merged just like @mcornella said.

fixes #4009, fixes #4010, fixes #4011, fixes #4018, closes #4019, fixes #4020, closes #4021 and fixes #4022

@webmaster128
Copy link

@kevin-lee afaik you can install Ubuntu packages on Travis. I think Travis runs Ubuntu 12.04 which does not have shellcheck. Shellcheck comes in utopic and trusty-backports. But it should be possible to get it from a PPA.

@kevin-lee
Copy link

@webmaster128 If so, I recommend Semaphore because it has Ubuntu 14.04 and you can even choose 12.04 if you want whereas Travis has only Ubuntu 12.04. In fact, I found this one. https://semaphoreci.com/docs/how-to-install-dependency-from-ppa.html So yeah there is a way to run shellcheck I think. 😄

@CodingFabian
Copy link

how about merging this? its not that there are not hundreds of users which are wondering why their config broke and a growing number of issues created.

@oryband
Copy link

oryband commented Jun 15, 2015

@adamantivm
Copy link

Merge please?

@oshybystyi oshybystyi mentioned this pull request Jun 15, 2015
This was referenced Jun 15, 2015
@kevin-lee
Copy link

@webmaster128 I was trying to set up CI for oh-my-zsh using shellcheck. Could you please tell me if what I did was correct?

shellcheck -s zsh **/*.zsh 

If so, that shows so many issues. I tested on Ubuntu 14.04 shellcheck version 0.3.3 and I also tried it on Mac OS X but shellcheck installed using Homebrew doesn't seem to support zsh although its version is 0.3.7.

Anyway, it shows so many issues so I'm not sure if we can simply have CI for this.

@webmaster128
Copy link

@kevin-lee The command looks good. Version 0.3.3 should be good for now.

Try this one to get things started:

find ./ -not -path "./plugins/*" -name "*.zsh" -exec shellcheck -s zsh --exclude=SC2034 {} \;

This excludes the plugins directory and SC2034 (unused variables).

ncanceill referenced this pull request Jun 15, 2015
Only load url-quote-magic if it is available.
robbyrussell added a commit that referenced this pull request Jun 15, 2015
Add missing space causing parse error.
@robbyrussell robbyrussell merged commit ab18795 into ohmyzsh:master Jun 15, 2015
@robbyrussell
Copy link
Member

My apologies. I got a little merge happy after a few glasses of wine last night.

👎

@tehmaspc
Copy link
Contributor

happy dance; that was quick; funny to see all the duplicate PRs :)

thanks!

@leepa
Copy link

leepa commented Jun 15, 2015

@robbyrussell thanks for merging - however it highlights an issue. There needs to be people able to merge outside of your time zone. The popularity of this has meant my entire working day this was broken. Now, I'm able to patch/fix things myself - however lots won't.

@oshybystyi
Copy link
Contributor

+1 for @leepa suggestion

@oryband
Copy link

oryband commented Jun 15, 2015

👍 for @leepa . Suggestions:

  1. move oh-my-zsh to an organization with multiple collaborators
  2. travis tests (or other CI options)
  3. coveralls to make the community push towards better coverage

bonus tip: don't drink and merge

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

Successfully merging this pull request may close these issues.

None yet