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

Update HttpInlet error with correct pip install command #5

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

cdpierse
Copy link
Collaborator

  • The error currently asks the user to 'pip install databay[HttpInlet]'
    this won't work for installing a package with optional depedencies.
  • This tiny PR adds quotation marks that are needed to correctly install
    aiohttp.

Also great job on this project. I'm really enjoying what you've done so far!

- The error currently asks the user to 'pip install databay[HttpInlet]'
this won't work for installing a package with optional depedencies.
- This tiny PR adds quotation marks that are needed to correctly install
aiohttp.
@Voyz
Copy link
Owner

Voyz commented Aug 23, 2020

Welcome @cdpierse ! Thank you for the first contribution to Databay🎉!!

Just before we go ahead with merging, would you mind if I ask you for the source of your contribution? I've looked into this, and I'm a bit over the fence. There are some sources which suggest that including the double quotes like you suggest is necessary - for instance this SO answer (although it suggest single quotes which don't always work). However, I also found some arguments against it:

  • This packaging.python.org page contains examples that don't use quotes: pip install SomePackage[PDF]
  • This PyPA documentation, under example 7 showcases the usage without quotes: pip install SomePackage[PDF,EPUB]
  • I've successfully used the command without quotes a variety of times

Now, using the method you propose also works - I'm just curious as to whether there's a good reason to suggest it as the default.

  • Would it be that without using quotes the install fails for you? Which Python version are you on if that's the case?
  • Would you have some source that discusses the advantage of one method over another? I can't find anything on that topic.

Totally happy to merge this in if not using the double quotes is an inferior method or breaks on your end - just trying to understand what is the case! Thanks for baring with me on this 😊

And once again thanks for participating and for the kind words! Really happy to hear you like this project 😅

@Voyz Voyz requested review from Voyz and removed request for Voyz August 23, 2020 05:43
@cdpierse
Copy link
Collaborator Author

Hey @Voyz,

Of course no problem, I probably should have gone into more detail. I originally tested this with Python 3.7.7 and noticed I was getting - zsh: no matches found: databay[HttpInlet] back as a response whenever I tried to install. I had done extra deps installs before and had always thought it required quotes of some kind in order to execute. I tried it out with a couple more python environments to be sure on my machine and got the same back.

I think this SO post has what I've seen to be best guess at what's going on.

In the comments section on the question part of the post one of the users mentions that the brackets are to be used as a protection against accidental shell expansion of the install pattern.

So my guess with this is that it has to be with my choice of "zsh" as my default shell and I'm probably running into the exact issue mentioned by that user.

What shell are you using on you machine btw ? My guess would be that it's not zsh.

I think including some sort of quotes is probably the safest thing to do here to make sure that everyone can install correctly off that command.

What do you think ?

@Voyz
Copy link
Owner

Voyz commented Aug 23, 2020

That's a fantastic explanation - you are absolutely right @cdpierse. I'm not using Zsh and so in my shell that was not a problem. Frankly, it's the first time I encounter shell expansion, so I'm glad you brought it to my attention. In a way I am surprised the documentation I quoted earlier doesn't advise to use double quotes, seeing how this could be a common problem for multiple users.

Naturally in such case your contribution is the right way to go, thank you for proposing it! Could I bother you to introduce that same change to outlets/__init__.py within this same PR? This way we'll cover all cases in which this type of message is currently displayed to the user.

Once again, thank you for catching this issue and creating this first PR, great job! 😊

@cdpierse
Copy link
Collaborator Author

No problem at all. I've just added those additional changes now. It does seem surprising that the documentation isn't more explicit on what the best practice should be.

Thanks again on a great package, I'm looking forward to seeing how this develops and hopefully contributing more in the future 😄

@Voyz Voyz merged commit 1fada99 into Voyz:master Aug 24, 2020
@Voyz
Copy link
Owner

Voyz commented Aug 24, 2020

That's great, thank you for adding these in! You also correctly caught that MongoOutlet has a small instruction on top which should also be updated. That made me realise we should also update HttpInlet that has an analogous instruction there too. I went ahead and committed that small change to your branch directly so that I could merge this in right way.

Also, I noticed you added __pycache__ to .gitignore. That's fine let's push it through, I just thought I'd mention that adding this to your global gitignore on your machine will solve that problem for all repos, but sure it won't hurt to add it locally too.

Going ahead with a merge. Congrats on completing the first contributing PR for Databay! 😄 Please accept the happy dancing penguin as a reward:

Looking forward to more collaboration in the future! Any questions send them over and let me know how your experience goes.

Take care!
Voy

@cdpierse cdpierse deleted the pip-install-fix branch August 24, 2020 10:17
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.

2 participants