Skip to content

Verbose info configuration #2918

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

Closed
wants to merge 4 commits into from
Closed

Verbose info configuration #2918

wants to merge 4 commits into from

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Feb 23, 2013

No description provided.

@ghost
Copy link

ghost commented Feb 23, 2013

GH is warning about merge conflicts.

@cpcloud
Copy link
Member Author

cpcloud commented Feb 23, 2013

Hmm I branched off of upstream/master. How can I see those merge conflicts?

@jreback
Copy link
Contributor

jreback commented Feb 23, 2013

make sure you rebase off current master
some changes in frame.py today (and might be some small conflicts)

@ghost
Copy link

ghost commented Feb 23, 2013

I can deal with that if you run into troube, but it'll probably rebase without trouble.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2013

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

@cpcloud
Copy link
Member Author

cpcloud commented Feb 23, 2013

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?

@ghost
Copy link

ghost commented Feb 23, 2013

sure, just put it in a seperate commit "CLN: ...".

@ghost
Copy link

ghost commented Feb 23, 2013

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.

@ghost
Copy link

ghost commented Feb 26, 2013

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.

@cpcloud
Copy link
Member Author

cpcloud commented Feb 26, 2013

Ok. Sorry about that. I won't submit any more pull requests until I've spent some more time with git.

@cpcloud
Copy link
Member Author

cpcloud commented Feb 26, 2013

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.

@hayd
Copy link
Contributor

hayd commented Mar 5, 2013

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)?

@cpcloud
Copy link
Member Author

cpcloud commented Mar 6, 2013

@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?

@hayd
Copy link
Contributor

hayd commented Mar 6, 2013

@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 :) .)

@ghost
Copy link

ghost commented Mar 7, 2013

@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)
<ipython-input-6-d14dc426f1c2> in <module>()
----> 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
Copy link

ghost commented Mar 14, 2013

rebased and merged. Thanks @cpcloud.

@ghost ghost closed this Mar 14, 2013
@ghost ghost mentioned this pull request Mar 29, 2013
ghost pushed a commit that referenced this pull request Mar 29, 2013
@cpcloud cpcloud deleted the verbose-info branch April 17, 2013 22:50
This pull request was closed.
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.

3 participants