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

Fixes for %paste with special transformations #2015

Merged
merged 13 commits into from Jun 28, 2012
Merged

Conversation

fperez
Copy link
Member

@fperez fperez commented Jun 23, 2012

NOTE: ready for review/merge

I'm just creating the PR for now with one failing test, need to add a second failing test for the ? case, and then actually fix things...

Fix the fact that we use prefilter too aggressively in %paste, which results in applying single-line transformations inside of strings and function definitions.

Closes #1258.

Move two top-level functions that had been marked for refactoring as
methods long ago and which I missed in the big cell magics work.
@fperez
Copy link
Member Author

fperez commented Jun 23, 2012

I need to add a test for this as well:

def func():
   if True: #am i true?
       a = 3

if copied and pasted into the IPython interpreter via paste looks like this

[39]: paste
def func():
   if True: #am i true?
       a = 3
## -- End pasted text --
Object `if` not found.

@fperez
Copy link
Member Author

fperez commented Jun 26, 2012

Test results for commit 0c2eef0 merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib oct2py pygments pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

@fperez fperez mentioned this pull request Jun 26, 2012
@minrk
Copy link
Member

minrk commented Jun 26, 2012

I'm not well familiar with the prefilter part of the code, so I would ping @takluyver for a more authoritative review, but I tried to read carefully, and it seems solid (and thoroughly commented!).

One functional change this seems to make is that this now dedents pasted text, which the previous implementation did not. Did you add a test for pasting an indented block?

@fperez
Copy link
Member Author

fperez commented Jun 26, 2012

Thanks, @minrk; I'll wait for @takluyver's careful eyes.

BTW, the old code did dedent as well, so nothing has changed on that front (well, the dedent is now unconditional and before it was omitted if a storage name was given; minor change in the name of simplifying an otherwise very spaghetti logic). So that means we do have already good tests in place for indented input (most of them, in fact, start indented).

@minrk
Copy link
Member

minrk commented Jun 26, 2012

Test results for commit 0c2eef0 (can't merge cleanly)
Platform: darwin

  • python2.6: OK (libraries not available: cython matplotlib oct2py pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: OK (libraries not available: oct2py wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib oct2py pymongo qt rpy2 wx wx.aui)

Not available for testing: python3.1

@minrk
Copy link
Member

minrk commented Jun 26, 2012

okay, great. I saw dedent code added, and missed its removal, and there was a comment that said "without dedenting" that you removed, so I thought it was changed behavior.

@fperez
Copy link
Member Author

fperez commented Jun 26, 2012

Yes, that was just indicating that the dedenting used to be optional, I made it global to simplify the unnecessarily branchy logic it led to.

@takluyver
Copy link
Member

Just a heads up: I'm going to a conference on Thursday, and there's stuff to prepare for that. I'll try to have a look at this one tomorrow, but if I don't get round to it, I'll be busy for a couple of weeks. Sorry about this, I know I'm disappearing just when we want all hands on deck for the release.

@fperez
Copy link
Member Author

fperez commented Jun 26, 2012

No worries. If it comes to it we'll merge it, as it's well tested and does fix a known bug (with specific tests for it). But I'll leave it open til tomorrow in case you do have a chance to give it a look.

Have fun at your conference!


_ip.user_ns['run'] = run
_ip.user_ns['runf'] = runf
Copy link
Member

Choose a reason for hiding this comment

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

Some of this code is using _ip, while most of it seems to be using ip. They're equivalent, but it would be cleaner to use one consistently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 85e9d88, thanks.

@takluyver
Copy link
Member

I haven't had time to delve into it much, but from what I can see it looks OK. I'd like proper tests for the new utils functions, but that shouldn't hold this up now.

@fperez
Copy link
Member Author

fperez commented Jun 28, 2012

@takluyver, there's only two new utilities and there are tests for both that I think cover reasonably what they do. What else would make them 'proper'?

@fperez
Copy link
Member Author

fperez commented Jun 28, 2012

Since this fixes a real bug and it looks like we're otherwise good, I'm merging it now. The only remaining point is the question of 'proper' tests, but I know @takluyver has a trip coming up, so that may have to wait. If we clarify that point later on as really needing extra tests (I'm not sure I see the need), we can always add those post-release.

fperez added a commit that referenced this pull request Jun 28, 2012
Fixes for %paste with special transformations.

Fix the fact that we use prefilter too aggressively in `%paste`, which results in applying single-line transformations inside of strings and function definitions.

Closes #1258.
@fperez fperez merged commit 11b77df into ipython:master Jun 28, 2012
@takluyver
Copy link
Member

Oh, I somehow missed the unit tests, and I thought they only had doctests. I see now that they had proper tests all along.

@fperez
Copy link
Member Author

fperez commented Jul 4, 2012

OK, we're good here then.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fixes for %paste with special transformations.

Fix the fact that we use prefilter too aggressively in `%paste`, which results in applying single-line transformations inside of strings and function definitions.

Closes ipython#1258.
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.

Magic %paste error
3 participants