Skip to content

WIP: Add support for values to query#389

Open
gregorjerse wants to merge 1 commit intogenialis:masterfrom
gregorjerse:feature/values
Open

WIP: Add support for values to query#389
gregorjerse wants to merge 1 commit intogenialis:masterfrom
gregorjerse:feature/values

Conversation

@gregorjerse
Copy link
Contributor

Just a suggestion.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

A work-in-progress PR to add support for querying specific fields from resources.

  • Extracts logic for fetching data into a new _fetch_data method.
  • Adds a new values method to support field-specific queries and reduce data transfer.
  • Adjusts the _fetch method to use _fetch_data and update caching accordingly.
Comments suppressed due to low confidence (2)

src/resdk/query.py:250

  • Ensure that 'count' is always initialized in _fetch_data. If items is neither a dict with 'results' nor a list when self._limit is not None, 'count' could remain undefined. Consider initializing 'count' with a default value before the conditions.
if isinstance(items, dict) and "results" in items:

src/resdk/query.py:233

  • [nitpick] The introduction of _fetch_data alongside _fetch may be slightly confusing. Consider renaming _fetch_data to something that clearly differentiates its purpose from _fetch, for example, _retrieve_data.
def _fetch_data(self):

Copy link
Contributor

@JureZmrzlikar JureZmrzlikar left a comment

Choose a reason for hiding this comment

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

I like the idea. A new method on the Query makes it more explicit that you are not receiving the resource objects, but just a dict of requested values. In this regard it seems better API choice than using fields=[...] argument in .e.g. filter() method. Let's keep this going with replacing the occurrence of ResolweQuery.filter(fields=[..]) where appropriate.

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.

2 participants