Skip to content

Add _encode_url to improve readability of url construction #79

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 8 commits into from
Closed

Add _encode_url to improve readability of url construction #79

wants to merge 8 commits into from

Conversation

femtotrader
Copy link
Contributor

A partial fix of #78

@jorisvandenbossche
Copy link
Member

Small remark: the urlencode from pandas you use, is in pandas only used in data.py, so I think it is better to put it here instead of importing it from pandas (and it is just an import from urllib actually: https://github.com/pydata/pandas/blob/df23f918cecbaf755cfdfc559a9ae98131fb390b/pandas/io/common.py#L18)

@femtotrader
Copy link
Contributor Author

So you think we should also have to maintain Python 2 and 3 compatibility here ?

@TomAugspurger
Copy link

Yes.

@femtotrader
Copy link
Contributor Author

That's done. I wonder why not using six (in Pandas also)

@jreback
Copy link
Contributor

jreback commented Aug 24, 2015

Return encoded url with parameters
"""
s_params = urlencode(params)
if len(s_params)!=0:
Copy link
Member

Choose a reason for hiding this comment

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

if s_params:

@hayd
Copy link
Member

hayd commented Aug 27, 2015

A previous idea was to rip out part of pandas.compat into another lib, as some older pandas have in compat is slightly different. That said, we're not using many compat features at all so could just add the few we are to util/_util. :/

@femtotrader
Copy link
Contributor Author

I think I will need some help about this PR.

@hayd
Copy link
Member

hayd commented Aug 27, 2015

to squash

git rebase -i head~8 # squash 8 commits
# change all but the first "pick" to "s", :wq
# clean up the commit message, :wq

@femtotrader
Copy link
Contributor Author

Some commit shouldn't be here

pick bb74632 TST: Fix test fred for new number of datapoints.
pick 8734d5c TST: pramga: no cover where appropriate
pick 6dea000 CLN: Add NotImplementedError to DataReader
pick e5b4039 CLN: Unnecessary try: block removed from Options
pick df405e2 DOC: update link to docs to point to actual docs
pick d6e10a1 CLN: Split into subpackages
pick 2044d8a Add _encode_url to improve readability of url construction
pick a9ce20e TST: Remove extra 'test_get_quote_string'.
pick 91e5c2d Add _encode_url to improve readability of url construction
pick b934e26 Add _encode_url to improve readability of url construction
pick 0f38c70 Update utils.py
pick 2b072c8 Add urlencode with Python 2/3 compatibility
pick 8084c3d if s_params

@femtotrader
Copy link
Contributor Author

I did

pick 2044d8a Add _encode_url to improve readability of url construction
squash 91e5c2d Add _encode_url to improve readability of url construction
squash b934e26 Add _encode_url to improve readability of url construction
squash 0f38c70 Update utils.py
squash 2b072c8 Add urlencode with Python 2/3 compatibility
squash 8084c3d if s_params

but it raises

error: could not apply 2044d8a... Add _encode_url to improve readability of url construction

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 2044d8a29dae7f5bf92a5a4c1eb657d50103a47f... Add _encode_url to improve readability of url construction

@hayd
Copy link
Member

hayd commented Aug 27, 2015

That's as you have merge commits, tip: don't use git merge.

Maybe git reset head~2 and then recommit.

@davidastephens
Copy link
Member

Closed via #83

@femtotrader
Copy link
Contributor Author

Thanks David for your help. You are my Git Guru ;-)

@femtotrader femtotrader deleted the url_encode_cleanup branch September 26, 2015 19:32
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.

6 participants