Issue 129 #251

Closed
wants to merge 6 commits into
from

Projects

None yet

2 participants

@takluyver
Member

(https://github.com/ipython/ipython/issues/issue/129/)

When a %psearch or equivalent search attempts to find the attributes of a dictionary, it instead looked at the items in the dictionary. This leads to unexpected results, and error messages if the dictionary has non-string keys. The simplest test case:

a = {1:"a"}
%psearch a.*

This branch adds a unit test for this situation (which originally fails), then fixes it. I then also simplified the IPython.utils.wildcard module somewhat.

@fperez
Member
fperez commented Feb 22, 2011

This looks great, thanks! My only suggestion: perhaps extending the test suite to have a case that triggers the try/except clause you added in dict_dir? Thanks for commenting it so clearly, but with code like that, it's always a good idea to add a test that would fail if someone ever makes a change that removes that kind of logic.

Obviously with such a good comment in place nobody would explicitly delete that code, but what can happen is that a refactoring effectively changes the logic and the precaution you put in place is sidestepped. Having a test case that would fail if that happens will serve as a tripwire for the problem from ever surfacing.

Otherwise great job! If you can add this little improvement, merge away when done. Thanks :)

@fperez
Member
fperez commented Feb 22, 2011

ps - remember that when a commit fixes an issue, like:

takluyver@2818a76

you can use at the end a line

Closes gh-129.

That way when the commit ever gets merged, the issue is closed automatically and there's a link between bug and commit.

Alternatively if you don't want to take that step too early (because you think it's possible that during review/development things may change), then simply add the 'closes gh-N' line when wrapping things up in one of the final commits, when you're confident it will go in.

I tend to forget closing bugs later on, so I use the gh- line a lot. Though I suspect you're better than me in that regard :)

@takluyver
Member

It's not actually my code or comment, I refactored it out of Namespace.__init__ ;-) I'll attempt to write a test, although I'm not entirely clear on how to trigger it. Time for some code digging.

Aside: I think github's diff view could do a better job where I've indented/dedented lines without changing them.

Thanks for the tip on Closes gh-. I'd seen it, but I wasn't quite sure whether to use it on an original commit or on merge.

@takluyver
Member

Does that look sufficient? I've checked that it passes, of course.

@fperez
Member
fperez commented Feb 23, 2011

Perfect, merge away!

Thanks for the great work!

@takluyver
Member

Merge branch 'issue-129'

closed by 26fb955
closed by 26fb955

@markvoorhies markvoorhies pushed a commit to markvoorhies/ipython that referenced this pull request Apr 21, 2011
@takluyver takluyver Merge branch 'issue-129'
closes gh-129
closes gh-251
26fb955
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@takluyver takluyver Merge branch 'issue-129'
closes gh-129
closes gh-251
11328bd
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment