Skip to content

Add shell detection - #6

Merged
ofek merged 19 commits into
masterfrom
ofek/detect
Jul 13, 2019
Merged

ofek merged 19 commits into
masterfrom
ofek/detect

Conversation

@ofek

@ofek ofek commented May 13, 2019

Copy link
Copy Markdown
Owner

Resolves #3

@ofek
ofek force-pushed the ofek/detect branch 4 times, most recently from d0eb83c to 8a77ce8 Compare May 13, 2019 06:45
@ofek

ofek commented May 13, 2019

Copy link
Copy Markdown
Owner Author

@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

jaraco commented May 13, 2019 •

Copy link
Copy Markdown

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")

Comment thread userpath/shells.py Outdated
locations = location.split(pathsep)

if front:
contents = '\n'.join('$PATH.insert(0, r"{}")'.format(location) for location in reversed(locations))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May I suggest {!r} instead of r"{}"?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Much nicer, thanks!

@ofek

ofek commented May 28, 2019

Copy link
Copy Markdown
Owner Author

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

  • Implemented the ability to select particular shells to modify, defaulting to auto-detection, and if that fails, to sh and bash
  • We now require https://github.com/nir0s/distro as a dependency to handle wrong shell #3 (comment) and other issues like it in future
  • Made integration tests via Docker for the login & non-login version of every shell, with full support for code coverage. Currently, we just test Debian (Stretch) but adding other Dockerfiles would be trivial

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

@jaraco

jaraco commented Jun 21, 2019

Copy link
Copy Markdown

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

AlJohri commented Jun 21, 2019

Copy link
Copy Markdown

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

@ofek
ofek merged commit e322af6 into master Jul 13, 2019
@ofek
ofek deleted the ofek/detect branch July 13, 2019 19:28
@ofek

ofek commented Jul 13, 2019

Copy link
Copy Markdown
Owner Author

1.2.0 is now on PyPI 😄

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

@ofek ofek mentioned this pull request Jul 18, 2019
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.

wrong shell

3 participants