Skip to content
This repository has been archived by the owner on Nov 7, 2021. It is now read-only.

Boolean parameter refresh is broken #73

Open
neumond opened this issue Nov 22, 2016 · 10 comments
Open

Boolean parameter refresh is broken #73

neumond opened this issue Nov 22, 2016 · 10 comments

Comments

@neumond
Copy link

neumond commented Nov 22, 2016

You can clearly see here conversion of parameter to boolean type.
https://github.com/aio-libs/aioes/blob/master/aioes/client/__init__.py#L133
Then it fails here
https://github.com/aio-libs/yarl/blob/master/yarl/__init__.py#L673

Considering this, there's no way to use refresh parameter at all.

@neumond
Copy link
Author

neumond commented Nov 22, 2016

Monkeypatch workaround

    import yarl
    original_with_query = yarl.URL.with_query

    def new_with_query(self, *args, **kwargs):
        if kwargs:
            query = kwargs
        elif len(args) == 1:
            query = args[0]
        if isinstance(query, Mapping) and 'refresh' in query and isinstance(query['refresh'], bool):
            query['refresh'] = '1' if query['refresh'] else '0'
        return original_with_query(self, *args, **kwargs)

    yarl.URL.with_query = new_with_query

@asvetlov
Copy link
Contributor

asvetlov commented Dec 7, 2016

Would you provide a patch which fixes aioes, without yarl monkeypatching?

@neumond
Copy link
Author

neumond commented Dec 8, 2016

There're two ways of solving this problem: modifying yarl for bool support and replacing bool parameters everywhere is aioes. What can you consider correct?

@asvetlov
Copy link
Contributor

asvetlov commented Dec 8, 2016

The second one

DLizogub added a commit to DLizogub/aioes that referenced this issue Jan 16, 2017
…arl raise 'Invalid variable type' TypeError
@wintamute
Copy link

Any plans on merging this and doing a new release soon?

@neumond
Copy link
Author

neumond commented Feb 16, 2017

@wintamute I would prefer to be able to convert from bool instead passing as is. refresh=True looks more reasonable than refresh=1.

@wintamute
Copy link

wintamute commented Feb 16, 2017

@neumond I agree, I was referring to the commit above (DLizogub@77c4d56)
Edit: I just checked that commit again, my bad, the fix should be to convert here to str instead of passing as is so that yarl doesn't complain later on. I didn't try it I admit.
Using a local modified version doesn't really help me since I need a version the build system can just download.

@kr41
Copy link

kr41 commented Mar 1, 2017

A less hackish monkey-patch to workaround this issue as well as #112

from aioes.connection import Connection

__original_perform_request = Connection.perform_request

def perform_request(self, method, url, params, body):
    url = url.lstrip('/')  # Fixes issue #112
    if params:
        for key, value in params.items():
            if isinstance(value, bool):
                params[key] = str(value).lower()
    return __original_perform_request(self, method, url, params, body)

Connection.perform_request = perform_request

@popravich
Copy link
Contributor

Hey, guys! Please check this out with latest master!

@haizaar
Copy link

haizaar commented Apr 2, 2017

Looks very much so. Many thanks for giving some love to ES5 support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants