Don't transform function calls on IPyAutocall objects #1121

Merged
merged 4 commits into from Dec 9, 2011

Conversation

Projects
None yet
4 participants
@takluyver
Member

takluyver commented Dec 7, 2011

Closes #1117.

@bfroehle

This comment has been minimized.

Show comment
Hide comment
@bfroehle

bfroehle Dec 7, 2011

The lstrip isn't necessary.... or at least we don't use it later on in the_rest.startswith('[')

The lstrip isn't necessary.... or at least we don't use it later on in the_rest.startswith('[')

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 7, 2011

Owner

Good point.

Owner

takluyver replied Dec 7, 2011

Good point.

@bfroehle

This comment has been minimized.

Show comment
Hide comment
@bfroehle

bfroehle Dec 7, 2011

Contributor

Anyway, looks fine to me. The logic in this section is terribly convoluted, the best I could do to disentangle it was something like

            if force_auto:
                # Don't rewrite if it is already a call.
                do_rewrite = not the_rest.startswith('(')
            else:
                if not the_rest:
                    # We only apply it to argument-less calls if the autocall
                    # parameter is set to 2.
                    do_rewrite = self.shell.autocall == 2
                elif the_rest.startswith('[') and hasattr(obj, '__getitem__'):
                    # Don't autocall in this case: item access for an object
                    # which is BOTH callable and implements __getitem__.
                    do_rewrite = False
                else:
                    do_rewrite = True

            # Figure out the rewritten command
            if do_rewrite:
                if the_rest.endswith(';'):
                    newcmd = '%s(%s);' % (ifun.rstrip(),the_rest[:-1])
                else:
                    newcmd = '%s(%s)' % (ifun.rstrip(), the_rest)
            else:
                newcmd = '%s %s' % (ifun,the_rest)

Contributor

bfroehle commented Dec 7, 2011

Anyway, looks fine to me. The logic in this section is terribly convoluted, the best I could do to disentangle it was something like

            if force_auto:
                # Don't rewrite if it is already a call.
                do_rewrite = not the_rest.startswith('(')
            else:
                if not the_rest:
                    # We only apply it to argument-less calls if the autocall
                    # parameter is set to 2.
                    do_rewrite = self.shell.autocall == 2
                elif the_rest.startswith('[') and hasattr(obj, '__getitem__'):
                    # Don't autocall in this case: item access for an object
                    # which is BOTH callable and implements __getitem__.
                    do_rewrite = False
                else:
                    do_rewrite = True

            # Figure out the rewritten command
            if do_rewrite:
                if the_rest.endswith(';'):
                    newcmd = '%s(%s);' % (ifun.rstrip(),the_rest[:-1])
                else:
                    newcmd = '%s(%s)' % (ifun.rstrip(), the_rest)
            else:
                newcmd = '%s %s' % (ifun,the_rest)

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 8, 2011

Member

Thanks, Brad. I've tested that, and committed it here.

Member

takluyver commented Dec 8, 2011

Thanks, Brad. I've tested that, and committed it here.

@bfroehle

This comment has been minimized.

Show comment
Hide comment
@bfroehle

bfroehle Dec 8, 2011

Oops, not sure what happened here.

Oops, not sure what happened here.

@bfroehle

This comment has been minimized.

Show comment
Hide comment
@bfroehle

bfroehle Dec 8, 2011

Need to revert this change as well, as the tests expect 'autocallable ()'. My bad.

Need to revert this change as well, as the tests expect 'autocallable ()'. My bad.

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 8, 2011

Owner

It's alright, it's a change I made (I should really have done a separate commit for it, but I wanted to go to bed).

The difference is that previously, if it decided not to rewrite the line, it simply used string formatting: newcmd = '%s %s' % (ifun,the_rest), which forced a single space in there. I've told it to use the default prefilter handler (normal_handler) instead. I think the new behaviour is better - when we're not rewriting it, I'd rather not change the input at all.

Owner

takluyver replied Dec 8, 2011

It's alright, it's a change I made (I should really have done a separate commit for it, but I wanted to go to bed).

The difference is that previously, if it decided not to rewrite the line, it simply used string formatting: newcmd = '%s %s' % (ifun,the_rest), which forced a single space in there. I've told it to use the default prefilter handler (normal_handler) instead. I think the new behaviour is better - when we're not rewriting it, I'd rather not change the input at all.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Dec 8, 2011

Member

I don't pretend to understand the slightly weird previous logic, but the replacement certainly looks clean and sensible, nice work!

Member

minrk commented Dec 8, 2011

I don't pretend to understand the slightly weird previous logic, but the replacement certainly looks clean and sensible, nice work!

takluyver added a commit that referenced this pull request Dec 9, 2011

Merge pull request #1121 from takluyver/i1117
Don't transform function calls on IPyAutocall objects

@takluyver takluyver merged commit 7ea3e2f into ipython:master Dec 9, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Dec 9, 2011

Member

Merged, to keep the queue moving. Tests are passing - if you spot any issues with this, I'll do my best to respond to them quickly.

@fperez - tagged so you're aware of this.

Member

takluyver commented Dec 9, 2011

Merged, to keep the queue moving. Tests are passing - if you spot any issues with this, I'll do my best to respond to them quickly.

@fperez - tagged so you're aware of this.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Dec 10, 2011

Member

Thanks, @takluyver. Good call on keeping the queue moving, otherwise we'll drown. I had a look and don't see any problems, and with the tests added we should be in good shape.

Member

fperez commented Dec 10, 2011

Thanks, @takluyver. Good call on keeping the queue moving, otherwise we'll drown. I had a look and don't see any problems, and with the tests added we should be in good shape.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1121 from takluyver/i1117
Don't transform function calls on IPyAutocall objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment