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

ENH: expose option_context as a top-level API GH5618 #5752

Merged
3 commits merged into from Dec 31, 2013

Conversation

@ghost
Copy link

commented Dec 19, 2013

Moving `option_context` to toplevel rather then making `set_option` a context manager means a one line change instead of metaclasses and config_prefix subtlety and the rest of the ich. When a given approach to something explodes in complexity I'll [bravely, bravely run away](http://www.youtube.com/watch?v=BZwuTo7zKM8) every single time.

closes #5618
replaces #5625.

@jtratner, your points about config_prefix being broken are valid, feel free to pick up in the future
if you're so inclined.

cc @jseabold

@jtratner

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2013

Why introduce a new name into the name space when you can just add __enter__ and __exit__ methods to set_option? I hit the meta class because I was doing too many things at once...

@ghost

This comment has been minimized.

Copy link
Author

commented Dec 19, 2013

I didn't understand all the crud you had to work around until I tried it myself.
it makes the config_prefix stuff break and it makes the code feel like doing a summersault
while reciting shakespeare backwards while doing taxes. It just makes me want to stand back.

The devs have been fine with this distinction for a long while, why's it not good enough for users?

@ghost

This comment has been minimized.

Copy link
Author

commented Dec 20, 2013

My bad, I misinterpreted the failing tests. Hopefully all green now with set_option as context manager.
Corrected authorship as well.

@jtratner

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

Okay, sure. Just seems like you could do
s/set_option/_prefixed_set_option/g and then rename option_context to
set_option (and if you use it not as a contextmanager it's exactly the
same as set_option)
On Dec 19, 2013 6:05 PM, "y-p" notifications@github.com wrote:

I didn't understand all the crud you had to work around Until I tried it
myself.
it makes the config_prefix stuff breaks and the summersault while reciting
shakespeare
backwards and doing taxes feel of the other out just makes me want to
stand back.

The devs have been fine with this distinction for a long while, why's it
not good enough for users?


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

@ghost

This comment has been minimized.

Copy link
Author

commented Dec 20, 2013

Hmm, thought I did, I basically copy-pasted your code.
The docstrings for != set_option didn't work btw, the @Property thing doesn't work
as expected for functions. I had to put in a fix.

The option_context thing is just cleaner all around, but I think it's all there now.

@jtratner

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

Go with whatever you like best. option_context is a much better name for it.

@ghost

This comment has been minimized.

Copy link
Author

commented Dec 31, 2013

Opted for exposing option_context as top-level API. It's just a simpler solution.

ghost pushed a commit that referenced this pull request Dec 31, 2013
y-p
Merge pull request #5752 from y-p/PR_expose_option_context
Make set_option into context manager

@ghost ghost merged commit 1ebb31e into pandas-dev:master Dec 31, 2013

@ghost ghost deleted the PR_expose_option_context branch Dec 31, 2013

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.