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

partial fix for issue #678 #877

Merged
merged 2 commits into from Oct 18, 2011
Merged

partial fix for issue #678 #877

merged 2 commits into from Oct 18, 2011

Conversation

djv
Copy link

@djv djv commented Oct 14, 2011

partial fix for issue #678

The patch handles the case of:

In [4]: >>> a = [1,
   ...: ... 2]

In [5]: a
Out[5]: [1, 2]

but not

In [3]: >>> a = [1
   ...: ... ,2]
  File "<ipython-input-3-5b39dccea2ae>", line 2
    ... ,2]
     ^
SyntaxError: invalid syntax

which happens to be valid in python.

@takluyver
Copy link
Member

Thanks, Daniel. At a glance, that looks sensible, and I don't think I've seen any examples where the comma is after the newline, so that's probably not a big concern.

Just to note: we prefer if you don't merge master back into feature branches, since it complicates the commit graph. In general, it should be fine to just make the change and leave it until it's merged into master. If it ends up unable to merge cleanly, you can rebase it, do a forced push, and make a note on the pull request that you've done that.

@fperez
Copy link
Member

fperez commented Oct 17, 2011

@djv, let us know if you need a hand with the rebasing, so that your pull request contains only the intended changes and not master.

@djv
Copy link
Author

djv commented Oct 17, 2011

OK, I removed the commit with the merge.

@fperez
Copy link
Member

fperez commented Oct 17, 2011

Great, that's better. Now, the fix looks good, but in all such cases it's a good idea to add also a test that fails when the fix is not in place but passes once the fix is in. This will ensure that the problem doesn't recur in the future. You don't need to write up a new file, you can simply add a new test to the existing test_inputsplitter.py.

With a small test as indicated above, this will be good to go. Thanks a lot!

@djv
Copy link
Author

djv commented Oct 18, 2011

Added one test which exposes the bug.
Is there a way to run all tests for IPython? The wiki says that import IPython; IPython.test() should do it, but it doesn't run any of the tests from test_inputsplitter.py. I had to do python -m unittest IPython.core.tests.test_inputsplitter

@fperez
Copy link
Member

fperez commented Oct 18, 2011

Sorry, that's a lie in the wiki. Can you point me to where you found it so I can fix it? The way to run the tests is via

iptest

which is a wrapper around nosetests. You can pass any nose flag

iptest -vvs

is my standard way of running it; and you can run specific subtests:

iptest -vvs IPython.core.tests.test_inputsplitter:test_spaces

runs only the test_spaces function.

@djv
Copy link
Author

djv commented Oct 18, 2011

It's at http://ipython.org/ipython-doc/stable/development/testing.html#for-the-impatient-running-the-tests. I tried iptest as you suggested and it doesn't seem to go through IPython.core.tests.test_inputsplitter.

@fperez
Copy link
Member

fperez commented Oct 18, 2011

On Tue, Oct 18, 2011 at 12:37 AM, Daniel Velkov
reply@reply.github.com
wrote:

It's at http://ipython.org/ipython-doc/stable/development/testing.html#for-the-impatient-running-the-tests.

Ouch! OK, but I've now realized that I can make some small fixes to
IPython so that actually works, even though it doesn't right now. So
instead of removing that from the docs, I'm going to fix things up so
that works :)

I tried iptest as you suggested and it doesn't seem to go through IPython.core.tests.test_inputsplitter.

??? That's weird. Are you sure that you ran iptest in the same
environment that is finding your development copy of IPython? It's
possible you picked up the system-wide iptest and that it ran the test
suite for your system-installed IPython, not actually finding your
development tree.

iptest is actually just this code:

from IPython.testing.iptest import main; main()

so you could also run that at the command line if iptest is finding
something wrong.

@fperez
Copy link
Member

fperez commented Oct 18, 2011

On Tue, Oct 18, 2011 at 11:56 AM, Fernando Perez fperez.net@gmail.com wrote:

Ouch! OK, but I've now realized that I can make some small fixes to
IPython so that actually works, even though it doesn't right now.  So
instead of removing that from the docs, I'm going to fix things up so
that works :)

Actually I take that back: the line

python -c "import IPython;IPython.test()"

does work, or at least is supposed to in general, and it does work
on my system. That way of running the test suite doesn't support
individual test selection like iptest does, it's just for
whole-suite running, but it does produce identical results to iptest
on my system. Sorry for confusing you, I thought our docs were out of
date but in fact I just looked at the code, and everything seems fine
on my system.

If the above isn't working for you, can you please paste the full
output so we can have a look and try to understand what's going on?

@djv
Copy link
Author

djv commented Oct 18, 2011

The only way I could make it work is by running python setup.py develop. After that iptest is doing the right thing.

@fperez
Copy link
Member

fperez commented Oct 18, 2011

On Tue, Oct 18, 2011 at 12:39 PM, Daniel Velkov
reply@reply.github.com
wrote:

The only way I could make it work is by running python setup.py develop. After that iptest is doing the right thing.

Ah, yes: that makes sense, and it's consistent with the docs that say
that iptest only works after you have actually installed ipython
(develop is a form of installation).

So it looks like everything is in good shape.

@fperez
Copy link
Member

fperez commented Oct 18, 2011

Merged after a small fixup for clarity/optim at the end. Thanks!

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

3 participants