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

Virtualenv cell magic #1891

Closed
wants to merge 14 commits into from
Closed

Virtualenv cell magic #1891

wants to merge 14 commits into from

Conversation

fccoelho
Copy link

@fccoelho fccoelho commented Jun 9, 2012

Hi,

here is my virtualenv extension. It is very simple, and has a couple of tests to it (84% coverage). It requires a couple of envs to test, but if they are not present, it raises SkipTest.

Suggestion for improvement both of the extention or its testing are very welcome!

"""
if 'WORKON_HOME' not in os.environ:
return False
if os.path.exists(os.environ['WORKON_HOME'] + '/' + env):
Copy link
Member

Choose a reason for hiding this comment

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

os.path.join ;-)

Copy link
Author

Choose a reason for hiding this comment

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

fixed!

Copy link
Member

Choose a reason for hiding this comment

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

Not fixed on this line, though!

Copy link
Author

Choose a reason for hiding this comment

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

fixed it.

@takluyver
Copy link
Member

For the tests: rather than relying on VEW being installed and active, we should set $WORKON_HOME to a temporary directory, make a virtualenv there and test against that. Also, rather than depending on different versions of Python, we should make the subprocess print(sys.path)*, and check that the temporary directory is in the output.

*brackets so it works on Python 2 or 3.

@fccoelho
Copy link
Author

I have Started on support for windows but didn't finish it. it will be pretty complicated since VEW doesn't support it.

@takluyver
Copy link
Member

OK, that's not quite what I meant with the tests. We shouldn't mess with user visible folders in the test suite. Have a look at IPython.utils.tempdir. In the setup() function, put something like:

workon_dir = TemporaryDirectory()
os.environ['WORKON_HOME'] = workon_dir.name

Then in teardown():

del os.environ['WORKON_HOME']
workon_dir.cleanup()

workon_dir will need to be a global variable (or make the tests a class and attach it to the instance). Really, teardown() should restore $WORKON_HOME to whatever it was previously, but that's probably not a major concern.

"""
Returns the correct activate command given the platform
"""
if sys.platform.startswith("linux"):
Copy link
Member

Choose a reason for hiding this comment

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

This should be the default behavior, as the most likely other platforms (OS X, freebsd, etc.) will all behave the same as Linux here, rather than "Platform not supported".

Copy link
Author

Choose a reason for hiding this comment

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

fixed that.

Copy link
Author

Choose a reason for hiding this comment

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

It's inactive for the time being but I intend to finish it in time for
release 0.14.
Em 11/06/2012 18:50, "Min RK" <
reply@reply.github.com>
escreveu:

  •    return
    
  • cmd = get_activate_cmd(line)
  • p = Popen(cmd, stdout=PIPE, stderr=PIPE, stdin=PIPE)
  • out, err = p.communicate(cell)
  • p.wait()
  • if err:
  •    print >> sys.stderr, err
    
  • return out

+_loaded = False
+
+def get_activate_cmd(line):

  • """
  • Returns the correct activate command given the platform
  • """
  • if sys.platform.startswith("linux"):

This should be the default behavior, as the most likely other platforms
(OS X, freebsd, etc.) will all behave the same as Linux here, rather than
"Platform not supported".


Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1891/files#r963238

def test_virtualenv_testenv():
ip = get_ipython()
result = ip.run_cell_magic('virtualenv', 'testenv', 'print("hello")')
nt.assert_equals('hello\n', result)
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this test do something to demonstrate that it's running in the virtualenv - like printing sys.path. Then we can get rid of the redundant test_virtualenv_pypy and test_virtualenv_python3.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

On Sun, Jun 17, 2012 at 10:01 PM, Thomas Kluyver <
reply@reply.github.com

wrote:

  • result = ip.run_cell_magic('virtualenv', 'pypyEnv', 'import
    sys;print\
  • (sys.version)')
    
  • nt.assert_true('PyPy' in result)

+def test_virtualenv_python3():

  • if not env_exists('py3'):
  •    raise SkipTest("Environment py3 not found, skipping test")
    
  • ip = get_ipython()
  • result = ip.run_cell_magic('virtualenv', 'py3', 'import sys;print \
  • (sys.version_info.major)')
  • nt.assert_equals('3\n', result)

+def test_virtualenv_testenv():

  • ip = get_ipython()
  • result = ip.run_cell_magic('virtualenv', 'testenv',
    'print("hello")')
  • nt.assert_equals('hello\n', result)

Let's make this test do something to demonstrate that it's running in the
virtualenv - like printing sys.path. Then we can get rid of the redundant
test_virtualenv_pypy and test_virtualenv_python3.


Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1891/files#r998914

Flávio Codeço Coelho

+55(21) 3799-5567
Professor
Escola de Matemática Aplicada
Fundação Getúlio Vargas
Praia de Botafogo, 190 sala 312
Rio de Janeiro - RJ
22250-900
Brasil

def test_virtualenv_testenv():
ip = get_ipython()
result = ip.run_cell_magic('virtualenv', 'testenv', 'import sys;print(sys.path)')
nt.assert_equals('tmp', os.path.split(eval(result)[1])[0].split(os.path.sep)[1])
Copy link
Member

Choose a reason for hiding this comment

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

Not a very robust test - all sorts of things can be in /tmp. And it's probably not called /tmp on Windows.

What about something like nt.assert_in(testenv_dir.name, result)?

@fperez
Copy link
Member

fperez commented Jun 26, 2012

@takluyver, @minrk: what's your take on this one? It seems there's a comment from @takluyver still pending improvement; is it otherwise in mergeable state? If so, is that small missing point a deal-breaker to merge it? Given this will be probably pretty popular, I'm willing to ask @fccoelho to improve the testing post-0.13 if need be, so that we can get it in now.

Thoughts?

@minrk
Copy link
Member

minrk commented Jun 26, 2012

There are several unaddressed comments here, and this is not even basically functional on any of my systems, due to the treatment of WORKON_HOME. I do not think it is ready to merge.

@fperez
Copy link
Member

fperez commented Jun 26, 2012

Got it. Then let's target this for 0.14, as I simply don't see it getting ready in a day or two (and us having the bandwidth to carefully review and test it). Thanks @minrk for that one...

@fccoelho
Copy link
Author

It's best this way. I don't have the time to address those issues right
now, but they are on my todo list.

thanks for the patience,

Flávio

On Tue, Jun 26, 2012 at 1:47 AM, Fernando Perez <
reply@reply.github.com

wrote:

Got it. Then let's target this for 0.14, as I simply don't see it getting
ready in a day or two (and us having the bandwidth to carefully review and
test it). Thanks @minrk for that one...


Reply to this email directly or view it on GitHub:
#1891 (comment)

Flávio Codeço Coelho

+55(21) 3799-5567
Professor
Escola de Matemática Aplicada
Fundação Getúlio Vargas
Praia de Botafogo, 190 sala 312
Rio de Janeiro - RJ
22250-900
Brasil

@fperez
Copy link
Member

fperez commented Jun 26, 2012

No worries, @fccoelho! We're very happy to have your contributions, and 0.14 will be out before you know it. It's OK to polish after merge, but since in this case it looks like it does need real work, re-targeting seems like the best policy. We're very happy to have you contributing to IPython, so I hope this bubbles to the top of your todo list before too long :)

@Carreau
Copy link
Member

Carreau commented Aug 24, 2012

Hi,

This PR is unactive for 2 month now. What is it's status ?

Thanks.

@fccoelho
Copy link
Author

It's inactive for now,

But I plan to finish it before the next Ipython's release.

Flávio

On Fri, Aug 24, 2012 at 6:31 AM, Bussonnier Matthias <
notifications@github.com> wrote:

Hi,

This PR is unactive for 2 month now. What is it's status ?

Thanks.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1891#issuecomment-7996725.

Flávio Codeço Coelho

+55(21) 3799-5567
Professor
Escola de Matemática Aplicada
Fundação Getúlio Vargas
Praia de Botafogo, 190 sala 312
Rio de Janeiro - RJ
22250-900
Brasil

@Carreau
Copy link
Member

Carreau commented Aug 25, 2012

Ok, no problem, take your time :-)

@bfroehle
Copy link
Contributor

@fccoelho:

I'm going to close this pull request for now. Please reopen it if and when you have time to complete the necessary changes.

Altenatively you might find it more convenient to just upload your extension to gist or similar and post it on the list of IPython exensions.

Thanks.

@bfroehle bfroehle closed this Sep 18, 2012
@fccoelho
Copy link
Author

fine.

On Tue, Sep 18, 2012 at 5:32 PM, Bradley M. Froehle <
notifications@github.com> wrote:

@fccoelho https://github.com/fccoelho:

I'm going to close this pull request for now. Please reopen it if and when
you have time to complete the necessary changes.

Altenatively you might find it more convenient to just upload your
extension to gist https://gist.github.com or similar and post it on the
list of IPython exensions http://wiki.ipython.org/Extensions_Index.

Thanks.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1891#issuecomment-8669327.

Flávio Codeço Coelho

+55(21) 3799-5567
Professor
Escola de Matemática Aplicada
Fundação Getúlio Vargas
Praia de Botafogo, 190 sala 312
Rio de Janeiro - RJ
22250-900
Brasil

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.

None yet

6 participants