Skip to content
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

Adding default service_urls list issue#165 #169

Closed
wants to merge 2 commits into from
Closed

Adding default service_urls list issue#165 #169

wants to merge 2 commits into from

Conversation

terryyz
Copy link
Collaborator

@terryyz terryyz commented May 10, 2020

@coveralls
Copy link

Coverage Status

Coverage increased (+2.7%) to 92.222% when pulling f0ce952 on terryzhuo110:issue_165 into afb17c7 on ssut:master.

@coveralls
Copy link

coveralls commented May 10, 2020

Coverage Status

Coverage decreased (-0.08%) to 89.453% when pulling cfee004 on terryyz:issue_165 into afb17c7 on ssut:master.

@ssut
Copy link
Owner

ssut commented Jun 10, 2020

Could you describe purpose of this? I think it would be a good idea to put this in constants if you just wanted to show possible service urls.

@terryyz
Copy link
Collaborator Author

terryyz commented Jun 10, 2020

Yes, this is for showing available service urls we can use. User can choose one of them they like. I've put the list in the constant. Do you mean it would be better if I set def __init__(self, service_urls=None, user_agent=DEFAULT_USER_AGENT, proxies=None, timeout=None): proxies=None, timeout=None, available_service_list=DEFAULT_SERVICE_URLS) ?

@ssut
Copy link
Owner

ssut commented Jun 10, 2020

@terryyz It seems self.available_service_list is not referenced from the Client's methods so I think it does not need to have that property (available_service_list) in Client, storing in constants is enough.

@terryyz
Copy link
Collaborator Author

terryyz commented Jun 10, 2020

Okay. I will close this and pull another request.

@terryyz terryyz closed this Jun 10, 2020
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.

None yet

3 participants