Verbose info configuration by cpcloud · Pull Request #2918 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
GH is warning about merge conflicts.
Hmm I branched off of upstream/master. How can I see those merge conflicts?
make sure you rebase off current master
some changes in frame.py today (and might be some small conflicts)
I can deal with that if you run into troube, but it'll probably rebase without trouble.
git br yourbranchname --set-upstream master
git pull --rebase
if u have conflicts u will need to edit the files
also u might want to rebase to 1 commit (or more)
git rebase -i
and either pick, squash, or fixup
when done
git push myfork yourbranch -f
I see that warnings is imported all over the place within a function. Is it ok that I remove those and import it at the top level?
sure, just put it in a seperate commit "CLN: ...".
on second thought, better not touch those.
a lot of those have to do with a single commit that will be reverted soon,
changing those will mean more manual work when the time comes.
This is the 2nd PR you've opened that needed massive surgery. Please show more
consideration towards the maintainers of projects you care enough about to participate.
Everyone finds git challenging at some point, but It's important to learn to use the
tools properly if you wish to contribute to OSS. There are plenty of tutorials on the web.
I've fixed the history and pushed a tidy commit to branch PR2918_cleanup on my fork.
Please fetch that, and use git reset --hard to get your PR branch on 0d38b7c.
On review, I see you've added an int type validator to the register_option call, and that
means it's impossible to set a None value, contradicting the documentation.
Please fix it, add a test for setting a None value and then force push to update the PR.
Ok. Sorry about that. I won't submit any more pull requests until I've spent some more time with git.
I just realized after looking at the surrounding tests that it looks like I attempted to reinvent the wheel in test_large_frame_repr. I didn't see reset_option or option_context until just now.
Also, you should consider getting travis-ci working, that way it's easy to see whether your code actually "works" :) ...well, actually, whether it fails.
Does your last comment mean you want this pull-request closed (and not merged)?
@hayd I would like for this to be merged if it doesn't break anything. Will it automatically show up on Github if the Travis build passes?
@cpcloud precisely, have a look at some of the other pull requests on here. (It passing won't necessarily mean it'll get merged though, but it helps :) .)
@cpcloud, validators must raise ValueError(msg) on error. Unfortunately, that
means you can't compose the builtin is_x via lambdas the way you attempted.
The idea is to force developers to provide a meaningful error message.
You can try:
In [3]: validtor=pd.core.config.is_instance_factory((int,type(None)))
In [4]: validtor(1)
In [5]: validtor(None)
In [6]: validtor(1.3)
ValueError Traceback (most recent call last) in () ----> 1 validtor(1.3)
/home/user1/src/pandas/pandas/core/config.pyc in inner(x) 700 def inner(x): 701 if not isinstance(x, _type): --> 702 raise ValueError("Value must be an instance of '%s'" % str(_type)) 703 704 return inner
ValueError: Value must be an instance of '(<type 'int'>, <type 'NoneType'>)'
Other then that I think this is fine to be merged, Not using option_context really doesn't matter.
Getting travis going would be good. You can just "git commit --amend" to fake
a new commit and force push to trigger a build on travis once it's enabled on your repo.
ghost pushed a commit that referenced this pull request
This pull request was closed.