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 basics.rst with $SHELL. #2671

Merged
merged 9 commits into from
Aug 10, 2018
Merged

Update basics.rst with $SHELL. #2671

merged 9 commits into from
Aug 10, 2018

Conversation

ionling
Copy link
Contributor

@ionling ionling commented Jul 29, 2018

State the role of $SHELL in pipenv shell command.

零件 added 2 commits July 29, 2018 21:23
State the role of `$SHELL` in `pipenv shell` command.
Update basics.rst with `$SHELL`.
@uranusjr
Copy link
Member

Hi, the shell detection went through a couple of changes in recent versions. The current logic works like this:

  1. Check if PIPENV_SHELL is set—use it if available.
  2. Detect the current shell automatically.
  3. If the detection fails, use SHELL or PYENV_SHELL.
  4. If either of the above two is set, use COMSPEC (added in master, will be in the next version).
  5. Give up

So setting SHELL is less useful in the current version. We encourage users to try to rely on auto shell detection, and set PIPENV_SHELL if that fails. Could you expand the section to reflect this behaviour better, so it can be more useful to more people? A link to the environment variable section would also be awesome.

Thanks for the work! Documentation contribution is the best ❤️❤️❤️❤️

@ionling
Copy link
Contributor Author

ionling commented Jul 31, 2018

Ok, I'll try it, as my English is poor. My draft may need your correction.

@ionling
Copy link
Contributor Author

ionling commented Jul 31, 2018

The 4th point, may be neither rather than either? As I googled COMSPEC, it's one of the environment variables used in DOS, OS/2 and Windows.

@uranusjr
Copy link
Member

Ah yes, sorry, typo :)

Add the description of `PIPENV_SHELL`
@ionling
Copy link
Contributor Author

ionling commented Aug 2, 2018

I just updated it, can you check it?

@uranusjr
Copy link
Member

uranusjr commented Aug 2, 2018

Hi, this looks good! Only one thing—can you switch the link to use Sphinx’s reference syntax instead?

Update link with Sphinx’s reference syntax.
@ionling
Copy link
Contributor Author

ionling commented Aug 2, 2018

En, I updated it again.

docs/basics.rst Outdated
@@ -326,6 +326,7 @@ You should do this for your shell too, in your ``~/.profile`` or ``~/.bashrc`` o

.. note:: The shell launched in interactive mode. This means that if your shell reads its configuration from a specific file for interactive mode (e.g. bash by default looks for a ``~/.bashrc`` configuration file for interactive mode), then you'll need to modify (or create) this file.

If you get something wrong with ``$ pipenv shell``, just check ``PIPENV_SHELL`` environment variable, ``$ pipenv shell`` will use it if available. For detail, see :ref:`configuration-with-environment-variables`.
Copy link
Member

Choose a reason for hiding this comment

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

If you get something wrong with

Can you change this to 'If you experience issues with`? Minor change but I think it's a tiny bit more natural.

just check PIPENV_SHELL environment variable

just check PIPENV_SHELL environment variable, $ pipenv shell just check the PIPENV_SHELL environment variable, which $ pipenv shell will use if available.

Minor tense changes mainly, thanks for contributing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@techalchemy techalchemy added Type: Documentation 📖 This issue relates to documentation of pipenv. PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. PR: awaiting-news The PR related to this issue is missing a news fragment and cannot be merged. Status: Awaiting Update ⏳ This issue requires more information before assistance can be provided. labels Aug 4, 2018
@ionling
Copy link
Contributor Author

ionling commented Aug 8, 2018

Just updated.

@techalchemy techalchemy removed the Status: Awaiting Update ⏳ This issue requires more information before assistance can be provided. label Aug 10, 2018
techalchemy added a commit that referenced this pull request Aug 10, 2018
@techalchemy techalchemy removed the PR: awaiting-news The PR related to this issue is missing a news fragment and cannot be merged. label Aug 10, 2018
@techalchemy techalchemy merged commit c79507b into pypa:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. Type: Documentation 📖 This issue relates to documentation of pipenv.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants