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

Update handling configuration items in template #2341

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion astroquery/template_module/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class Conf(_config.ConfigNamespace):
"""
Configuration parameters for `astroquery.template_module`.
"""
server = _config.ConfigItem(
url = _config.ConfigItem(
['http://dummy_server_mirror_1',
'http://dummy_server_mirror_2',
'http://dummy_server_mirror_n'],
Expand Down
37 changes: 26 additions & 11 deletions astroquery/template_module/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,30 @@
@async_to_sync
class TemplateClass(BaseQuery):

# `__init__()` allows the user to conveniently set the instance attributes
# to override the configuration items. The default attribute values should
# be falsy (`x` is called falsy if `bool(x)` is `False`) or `None`.
def __init__(self, url='', timeout=None):
self.url = url # A falsy default that cannot be mistaken for a valid value.
self.timeout = timeout # Use `None` as default if the falsy value could be valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure our users will all be familiar with the term "falsy" - maybe clarify, e.g., ("falsy" means in this case bool(url) is False)

Copy link
Member Author

Choose a reason for hiding this comment

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

The target audience for these comments are developers, but I added an explanation nonetheless.

Copy link
Contributor

Choose a reason for hiding this comment

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

our developers are not necessarily experienced, and in particular, not necessarily experienced in python (we have many people coming from Java backgrounds, for example)

Copy link
Member

Choose a reason for hiding this comment

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

But then "falsy" is not a python specific term :) Anyway, I don't think it matters either way. Also, in my experience, many new modules are taking another similar module as a template rather than the "template" one, so we may even want to rethink the purpose of the template.


# The private properties defined here allow the users to change the
# configuration values at runtime if the corresponding instance attributes
# have default values.

@property
def _url(self):
return self.url or conf.url

@property
def _timeout(self):
return conf.timeout if self.timeout is None else self.timeout

"""
Not all the methods below are necessary but these cover most of the common
cases, new methods may be added if necessary, follow the guidelines at
<http://astroquery.readthedocs.io/en/latest/api.html>
"""
# use the Configuration Items imported from __init__.py to set the URL,
# TIMEOUT, etc.
URL = conf.server
TIMEOUT = conf.timeout

# all query methods are implemented with an "async" method that handles
# making the actual HTTP request and returns the raw HTTP response, which
Expand Down Expand Up @@ -124,8 +139,8 @@ def query_object_async(self, object_name, get_query_payload=False,
return request_payload
# BaseQuery classes come with a _request method that includes a
# built-in caching system
response = self._request('GET', self.URL, params=request_payload,
timeout=self.TIMEOUT, cache=cache)
response = self._request('GET', self._url, params=request_payload,
timeout=self._timeout, cache=cache)
return response

# For services that can query coordinates, use the query_region method.
Expand Down Expand Up @@ -169,8 +184,8 @@ def query_region_async(self, coordinates, radius, height, width,
width)
if get_query_payload:
return request_payload
response = self._request('GET', self.URL, params=request_payload,
timeout=self.TIMEOUT, cache=cache)
response = self._request('GET', self._url, params=request_payload,
timeout=self._timeout, cache=cache)
return response

# as we mentioned earlier use various python regular expressions, etc
Expand Down Expand Up @@ -294,9 +309,9 @@ def get_image_list(self, coordinates, radius, get_query_payload=False,
request_payload = self._args_to_payload(coordinates, radius)
if get_query_payload:
return request_payload
response = self._request(method="GET", url=self.URL,
response = self._request(method="GET", url=self._url,
data=request_payload,
timeout=self.TIMEOUT, cache=cache)
timeout=self._timeout, cache=cache)

return self.extract_image_urls(response.text)

Expand All @@ -322,7 +337,7 @@ def extract_image_urls(self, html_str):
pass


# the default tool for users to interact with is an instance of the Class
# the default tool for users to interact with is a default instance of the Class
Template = TemplateClass()

# once your class is done, tests should be written
Expand Down