Added handling for v3 advanced segment ids which aren't just ints as of July 15th by jcbozonier · Pull Request #5271 · 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

Conversation22 Commits1 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 }})

pls put these tests in the existing test_ga.py file
pls hook up Travis (see contributing.md)

Ok. Updated tests... will check on Travis.

OK. Travis build passed as well.

Can you this down to one commit and rebase on master?

Also, does this break v2 support? (or is v2 no longer supported by Google?)

Sure! How can I do the squash given the current state of my branch? Do I need to issue a new pull request?

As far as v2 goes this does not break v2 support. There are unit tests to support those cases as well in my code.

Don't need a new PR, just squash and do git push --force

@jcbozonier one last thing - can you add a note to doc/source/release.rst that says you added support for v3 segment ids for GA? And then just do git commit --amend -C HEAD (amends your previous commit and keeps message) and push one more time, then I'll merge.

as an aside if you would like to they the exception messages when deps are not present and make a bit nicer

Added a note. Also, I wanted to add a nicer exception but I wasn't sure how to do it in a way that was 2.6 AND 3.2 compatible. The next enhancement I make I'll take another stab at it.

compatible exception is just:

raise ExceptionName("My error message"), whereas <= 2.6 syntax was raise ExceptionName, "error message". Is that what you mean?

the former version is 2.6 - 3.X compatible

Does it make more sense just to let it blow? Why even catch those exceptions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move this to the end of the section (b/c it's pretty much chronological) and add (:issue:'5271') to the end [but replace single quote with backticks], that'll make it end up with a link to this issue in the docs so someone who cares can find out more.

it's basically just something like:

try: import except ImportError as e: raise ImportError("read_ga requires Google's ga package: %s" % str(e))

Your call - I'm +1 if you want to add a nice error message but I'll merge this after you make that slight to change to the docs note if you don't want to add it.

I'm fine with adding that. To clarify, you mean to the split test file, right?

No - I think @jreback meant in the read_ga() method

I guess you'll need to rebase this - apparently there's a merge conflict with something else (probably another PR that was merged in the interim)

The old int based advanced segment ids are deprecated. The new segment ids are alphanumeric. I know from experimentation that their form includes alphanumeric and alphanumeric+hyphen. Updated code for both cases.

Consolidated tests and fixed broken references

In consolidating my test file with the preexisting GA test file, I found a broken dependency to reset_token_store. It looks like it was renamed to reset_default_token_store so I updated this file to point to that. Also added tests around formatting of segment ids to the GA query to this file.

CLN Removed debugging code I added.

jtratner added a commit that referenced this pull request

Oct 22, 2013

Added handling for v3 advanced segment ids which aren't just ints as of July 15th