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

Adapter for CKAN #214

Merged
merged 10 commits into from
Sep 5, 2023
Merged

Adapter for CKAN #214

merged 10 commits into from
Sep 5, 2023

Conversation

m-qlas
Copy link
Contributor

@m-qlas m-qlas commented Aug 31, 2023

No description provided.

@m-qlas m-qlas requested a review from Vixtir August 31, 2023 13:33

@property
def resources(self) -> list[dict]:
return self.data["resources"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Map Resources here, and remove that logic from adapter.py
Same time you can use cachedproperty it that property used several times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

f"Error during getting data from workspace {self.__host}: {e}"
) from e

async def get_organizations(self) -> list[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be better to fetch organization details here and return list of Organization?
You can use here gather to fetch information concurrently, because now in adapter.py in for loop you do it in sequential way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

async def get_organizations(self) -> list[str]:
url = "/api/action/organization_list"
resp = await self._get_request(url)
if resp and resp["success"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can dry that expression, because it used below many times in the same way

Copy link
Contributor

Choose a reason for hiding this comment

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

And may be add some logic if response was failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created the method is_response_successful and moved this check to the _get_request method. In case of failure, it will raise DataSourceError. Only for _post_request, this isn't added because not all files have structure included in CKAN, and in case of failure, we just want to return an empty list.

@Vixtir Vixtir merged commit 4aeb172 into main Sep 5, 2023
3 checks passed
@Vixtir Vixtir deleted the ckan branch September 5, 2023 10:40
@Vixtir
Copy link
Contributor

Vixtir commented Sep 7, 2023

Closed #205

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

Successfully merging this pull request may close these issues.

2 participants