Conversation
kryachkow
requested review from
Fedir-Yatsenko
and removed request for
ypldan
… changes to reader.py
| ) | ||
| from .updated_at_mixin import UpdatedAtMixin | ||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
__all__ doesn't add value here. Everything listed in it is already explicitly imported at the top of the file, so a from statgpt.common.data.common import * wildcard import would pick up exactly those names regardless — __all__ just duplicates the import list.
__all__ is useful when:
- A module defines names locally and you want to control which ones are public
- You want to exclude some imported names from wildcard exports
- The module uses
importstatements that bring in names you don't want re-exported
None of those apply here. Consider removing __all__.
There was a problem hiding this comment.
statgpt/common/data/proxy/sdmx_schemas/init.py
statgpt/common/data/proxy/v30/init.py
statgpt/common/data/proxy/init.py
…rganization and clarity
There was a problem hiding this comment.
Since this is now part of the basic package, let's think about what is part of the SDMX standard, and what is SDMX+? For SDMX+ schemas, I would name the file something like sdmx_plus_schemas.py. POST is definitely not part of the standard, at least not yet, but I believe Annotation is, so we can leave it in this file.
There was a problem hiding this comment.
Or there may be some hierarchy, such as:
sdmx_schemas/
├── __init__.py
├── sdmx_30.py
└── sdmx_plus.py
| return AsyncProxySdmxClient.from_config(self._config, auth_context, rate_limiter) | ||
|
|
||
| def _should_use_cache(self, allow_cached: bool) -> bool: | ||
| auth_enabled = getattr(self._config, "auth_enabled", False) |
There was a problem hiding this comment.
There is no such property in the ProxySdmx30DataSourceConfig class, and I'm not sure if it will appear in the near future.
| class ProxySdmx30DataSourceConfig(SdmxDataSourceConfig): | ||
| """Configuration for SDMX 3.0 proxy data sources that still use sdmx1 parsing.""" | ||
|
|
||
| versions: set[str] = Field( |
There was a problem hiding this comment.
Is this used anywhere?
| async def _get_dataset( | ||
| def _should_use_cache(self, allow_cached: bool) -> bool: | ||
| auth_enabled = getattr(self._config, "auth_enabled", False) | ||
| return allow_cached and not auth_enabled |
There was a problem hiding this comment.
Whenever possible, let's use the field directly rather than getattr. Or is there a reason why not to do the following way here?
def _should_use_cache(self, allow_cached: bool) -> bool:
return allow_cached and not self._config.auth_enabled| ) | ||
| key = {} if key is None else key | ||
| req_body_obj = QhAvailabilityRequestBody.get_from(key=key, params=params) | ||
| req_body_obj = PostAvailabilityRequestBody.get_from(key=key, params=params) |
There was a problem hiding this comment.
Is PostAvailabilityRequestBody supposed to be used in the QuantHub package? If so, perhaps we should move this into the SDMX+ schemas of base package? And give it a more general name?
There was a problem hiding this comment.
There may be some code duplication from the sdmx1 library, let's try to remove it.
2 participants
Applicable issues
Description of changes
This update enhances the functionality and data handling capabilities of the SDMX proxy system.
Checklist
Title of the pull request follows Conventional Commits specification
Deployed and tested in a Review environment.
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.