ofek / userpath

Cross-platform tool for adding locations to the user PATH, no elevated privileges required!
MIT License
151 stars 20 forks source link

Add shell detection #6

Closed ofek closed 4 years ago

ofek commented 5 years ago

Resolves #3

ofek commented 5 years ago

@AlJohri Would you mind testing zsh? @jaraco Would you mind testing xonsh?

I plan to add a Docker env for each shell to tests but I won't have time this week

jaraco commented 5 years ago

I'm getting an error when it runs, though it does seem to have added the expected lines:

userpath ofek/detect $ git-id                                                                                                                                                  
3cc42f8
userpath ofek/detect $ tox --notest                                                                                                                                            
GLOB sdist-make: /Users/jaraco/code/public/userpath/setup.py
python create: /Users/jaraco/code/public/userpath/.tox/python
python installdeps: pytest
python inst: /Users/jaraco/code/public/userpath/.tox/.tmp/package/1/userpath-1.1.0.zip
python installed: atomicwrites==1.3.0,attrs==19.1.0,Click==7.0,more-itertools==7.0.0,pluggy==0.11.0,py==1.8.0,pytest==4.5.0,six==1.12.0,userpath==1.1.0,wcwidth==0.1.7
___________________________________________________________________________________ summary ___________________________________________________________________________________
  python: skipped tests
  congratulations :)

userpath ofek/detect $ .tox/python/bin/python -m userpath append foo/bar                                                                                                       
An unexpected failure seems to have occurred.
userpath ofek/detect $ tail -n 3 ~/.xonshrc                                                                                                                                    

# Created by `userpath` on 2019-05-13 12:20:01
$PATH.append(r"/Users/jaraco/code/public/userpath/foo/bar")
ofek commented 5 years ago

@AlJohri @jaraco @cs01 So, here's what I've done:

I'd appreciate it if you could test this once more, then I'll merge and release!

jaraco commented 5 years ago

Looking good!

userpath ofek/detect $ git-id
6ace5e6
userpath ofek/detect $ tox --notest -qq
userpath ofek/detect $ .tox/python/bin/python -m userpath append foo/bar
Success!
userpath ofek/detect $ tail -n 3 ~/.xonshrc

# Created by `userpath` on 2019-06-21 20:28:07
$PATH.append('/Users/jaraco/code/public/userpath/foo/bar')

Couple of comments follow.

Should there be output for success? Unix conventions say don't emit anything on success, although maybe that's an old convention. You may be asked later to add a -q operator to suppress that output. I don't feel strongly about it.

At first, the help text had me thinking that the Windows behavior was unnecessarily special cased. In particular:

-a, --all-shells; Update PATH of all supported shells. This has no effect on Windows as that is the standard behavior.

This made me think that on Windows, it would attempt to modify the config of all these shells... then I realized that the reason "that is the standard behavior" means that environment settings always affect all shells, not that userpath will be modifying all supported shells. I'm not sure there's anything to do here, but I thought I'd share my momentary confusion.

Overall, no objections. Looks great to me.

AlJohri commented 5 years ago

I'll try and test this out early next week- apologies for the delay.

ofek commented 4 years ago

1.2.0 is now on PyPI 😄

@jaraco I incorporated your suggestions @AlJohri Can we proceed with the Homebrew PR now?