Add Vox environment manager to xonsh #675

moigagoo · 2016-02-12T08:37:13Z

Registered vox as a default alias.
Added the {env_name} prompt variable.
Added environment name indication to the default prompt.

gforsyth · 2016-02-12T11:58:40Z

xonsh/vox.py

+
+        print('Creating environment...')
+
+        vbuiltins.__xonsh_env__.create(env_path, with_pip=True)


vbuiltins?

Thanks a lot for the correction! It's supposed to be venv. When I autoreplaced ENV with builtins.xonsh_env, I totally overlooked this bit :-(

Fixed in moigagoo@b92969e

scopatz · 2016-02-12T16:55:28Z

Looks like there are conflicts now, probably from the new parser being merged in to master. If you could resolve these that would be awesome!

scopatz · 2016-02-12T16:56:54Z

Also, a fair amount of this seems to be reimplementing what argparse does. We should probably just move to argparse. There are a lot of examples in xonsh that use it already.

moigagoo · 2016-02-12T18:12:47Z

Conflict resolved, ready to merge.

I didn't quite understand your last comment. Do you mean that I reimplemented argparse functionality in Vox? Or are you talking about the new parser?

I can't see how Vox can be rewritten to utilize argparse since xonsh handles argument parsing already. If it were a standalone app, there would be a way to do it, but I'd still use a more high-level library.

scopatz · 2016-02-13T05:08:50Z

xonsh/vox.py

+    def new(name):
+        """Create a virtual environment in $VIRTUALENV_HOME with python3's ``venv``.
+
+        :param name: virtual environment name


Please use numpydoc style docstrings.

Fixed in moigagoo@60daf5e

scopatz · 2016-02-13T05:38:09Z

Thanks for resolving the conflicts, @moigagoo! And thanks for withstanding my many comments. In addition to those, it would be great if you could add in some autodoc hooks in the docs/ dir, since vox.py is a new file. Also, it'd be great if you added an entry to the CHANGELOG. People will want to know about this. :)

I didn't quite understand your last comment. Do you mean that I reimplemented argparse functionality in Vox?

Yes, this.

I can't see how Vox can be rewritten to utilize argparse since xonsh handles argument parsing already. If it were a standalone app, there would be a way to do it,

You could stick an ArgumentParser instance in Vox.__call__(). Argument parsers let you pass in an arglist manually. There are many examples of xonsh aliases that do this:

but I'd still use a more high-level library.

Tools like cliar are great. However, we are trying to maintain maximum protablility by relying only on the Python standard library and PLY as required runtime dependencies. (And I have thought about moving a copy of PLY into xonsh so that it could be basically standalone.) I consider vox of core value, so it would be nice if it didn't rely on optional dependencies :)

Also, note, that not using argeparse is not blocking this. It was just a suggestion. We have other many other aliases that don't use argparse, so it is a reasonable choice not to.

… of $HOME.

…he change.

scopatz · 2016-02-13T18:49:47Z

xonsh/environ.py

@@ -384,6 +387,8 @@ def xonshconfig(env):
        default='xonsh.environ.DEFAULT_TITLE'),
    'VI_MODE': VarDocs(
        "Flag to enable 'vi_mode' in the 'prompt_toolkit' shell."),
+    'VIRTUAL_ENV': VarDocs(
+        "Path to the currently active Python environment."),


The should have configurable=False, I would think.

moigagoo@15be794

scopatz · 2016-02-13T18:52:16Z

xonsh/vox.py

+                home_path = builtins.__xonsh_env__['USERPROFILE']
+
+            elif os.name == 'posix':
+                home_path = os.path.expanduser('~')


Sorry, not just this line, but this whole conditional can be replaced with the expanduser(). Rather than having the if-elif-else blocks, you can just have the one line.

Sorry if that wasn't clear.

I feel so stupid right now :-)

moigagoo@17cb4cb

I'm sure this was my bad explanation in the early morning :)

On Sat, Feb 13, 2016 at 1:58 PM Konstantin Molchanov <
notifications@github.com> wrote:

In xonsh/vox.py
#675 (comment):

+import builtins
+
+
+class Vox:

"""Vox is a virtual environment manager for xonsh."""

def init(self):

"""Ensure that $VIRTUALENV_HOME is defined and declare the available vox commands"""

if not builtins.**xonsh_env**.get('VIRTUALENV_HOME'):

if os.name == 'nt':

home_path = builtins.**xonsh_env**['USERPROFILE']

elif os.name

== 'posix':

home_path = os.path.expanduser('~')

I feel so stupid right now :-)

moigagoo/xonsh@17cb4cb
moigagoo@17cb4cb

—
Reply to this email directly or view it on GitHub
https://github.com/scopatz/xonsh/pull/675/files#r52831995.

Asst. Prof. Anthony Scopatz
Nuclear Engineering Program
Mechanical Engineering Dept.
University of South Carolina
Affiliated Professor of the University of Haifa
scopatz@cec.sc.edu
Office: (803) 777-7629
Cell: (512) 827-8239
Check my calendar
https://www.google.com/calendar/embed?src=scopatz%40gmail.com

scopatz · 2016-02-13T18:57:59Z

xonsh/vox.py

+        if os.name == 'nt':
+            bin_dir = 'Scripts'
+
+        elif os.name == 'posix':


You should probably import ON_WINDOWS and ON_POSIX from xonsh.tools and use them here, for consistency.

Fixed: moigagoo@1186637

scopatz · 2016-02-13T19:14:05Z

OK @moigagoo, let us know when you think that this is ready to go in!

moigagoo · 2016-02-13T19:15:01Z

I fixed all the issues you had reported except for the argparse one. Fingers crossed :-)

First, thanks a lot for investing your time in reviewing and commenting on my commits.

Second, I'd like to comment a bit on the argparse issue. I think it's a bit of an overkill in this case. Argparse has such an overcomplicated API, which, I believe, doesn't really pay off in this task. However, if you think it's the right thing to do, I'm ready to do it—xonsh is your project after all :-)

moigagoo · 2016-02-13T19:15:41Z

🎆

scopatz · 2016-02-13T19:18:30Z

First, thanks a lot for investing your time in reviewing and commenting on my commits.

Thank you for investing your time and effort in making xonsh better! Hopefully, that will continue :)

Second, I'd like to comment a bit on the argparse issue. I think it's a bit of an overkill in this case. Argparse has such an overcomplicated API, which, I believe, doesn't really pay off in this task. However, if you think it's the right thing to do, I'm ready to do it—xonsh is your project after all :-)

This is fine. As I said, I don't think argparse should block this since we have many other core utilities in xonsh that don't use it. Using argpare seems to be about 50%, so I don't think it is a big deal.

If you think it is ready, let me know and I'll merge it in.

moigagoo · 2016-02-13T19:19:55Z

If you think it is ready, let me know and I'll merge it in.

Sure, go ahead.

Add Vox environment manager to xonsh

scopatz · 2016-02-13T19:20:26Z

Thanks a million, @moigagoo!

Konstantin Molchanov added 6 commits February 11, 2016 11:47

Vox added to xonsh; vox command added to default aliases.

90b5cbe

ENV replaced with builtins.__xonsh_env__

2b3acfc

AttributeError during Vox loading fixed.

1c0692f

Added prompt variable to show environment name.

2f78d5b

Remaining ENV occurences replaced with builtins.__xonsh_env__.

37006fd

Added enrironment name to default prompt.

79754cd

gforsyth reviewed Feb 12, 2016
View reviewed changes

Konstantin Molchanov added 2 commits February 12, 2016 15:06

Vox: New: Typo fixed.

b92969e

Vox: New: Doctring updated.

7962fcd

scopatz added the feature label Feb 12, 2016

scopatz added this to the v0.3 milestone Feb 12, 2016

Merge branch 'master' of github.com:scopatz/xonsh

c60a7c7

scopatz reviewed Feb 13, 2016
View reviewed changes

moigagoo added 5 commits February 13, 2016 21:17

Doctrings converted to numpydoc format.

60daf5e

Vox methods renamed to avoid collisions with builtins.

89d7b75

Vox: Windows: os.path.expanduser now used to detect home path instead…

7488142

… of $HOME.

$VIRTUAL_ENV registered in xonsh enviroment; Vox updated to reflect t…

926b74b

…he change.

Docs: API docs for Vox added.

84b0973

scopatz reviewed Feb 13, 2016
View reviewed changes

Vox addition reflected in the changelog.

0a86cab

scopatz reviewed Feb 13, 2016
View reviewed changes

Vox: $VIRTUALENV_HOME check simplified.

17cb4cb

scopatz reviewed Feb 13, 2016
View reviewed changes

moigagoo added 3 commits February 13, 2016 22:01

Default value for $VIRTUAL_ENV set.

5bb1ea7

Vox: xonsh.tools used instead of os.name checks for consistency.

1186637

Environment: Default docs: VIRTUAL_ENV now has configurable=False set.

15be794

scopatz added a commit that referenced this pull request Feb 13, 2016

Merge pull request #675 from moigagoo/vox

f82d304

Add Vox environment manager to xonsh

scopatz merged commit f82d304 into xonsh:master Feb 13, 2016

moigagoo mentioned this pull request May 13, 2016

Virtualenv in xonsh shell #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Vox environment manager to xonsh #675

Add Vox environment manager to xonsh #675

moigagoo commented Feb 12, 2016

gforsyth Feb 12, 2016

moigagoo Feb 12, 2016

scopatz commented Feb 12, 2016

scopatz commented Feb 12, 2016

moigagoo commented Feb 12, 2016

scopatz Feb 13, 2016

moigagoo Feb 13, 2016

scopatz commented Feb 13, 2016

scopatz Feb 13, 2016

moigagoo Feb 13, 2016

scopatz Feb 13, 2016

scopatz Feb 13, 2016

moigagoo Feb 13, 2016

scopatz Feb 13, 2016

scopatz Feb 13, 2016

moigagoo Feb 13, 2016

scopatz commented Feb 13, 2016

moigagoo commented Feb 13, 2016

moigagoo commented Feb 13, 2016

scopatz commented Feb 13, 2016

moigagoo commented Feb 13, 2016

scopatz commented Feb 13, 2016


		print('Creating environment...')

		vbuiltins.__xonsh_env__.create(env_path, with_pip=True)

Add Vox environment manager to xonsh #675

Add Vox environment manager to xonsh #675

Conversation

moigagoo commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Feb 12, 2016

scopatz commented Feb 12, 2016

moigagoo commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Feb 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Feb 13, 2016

moigagoo commented Feb 13, 2016

moigagoo commented Feb 13, 2016

scopatz commented Feb 13, 2016

moigagoo commented Feb 13, 2016

scopatz commented Feb 13, 2016