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

feat: add dataset.statistics #621

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MFori
Copy link
Contributor

@MFori MFori commented Jan 1, 2025

Part of https://github.com/apify/apify-core/issues/18807 - Implementation of new API endpoint v2/datasets/{datasetId}/statistics

@MFori MFori added the t-console Issues with this label are in the ownership of the console team. label Jan 1, 2025
@MFori MFori self-assigned this Jan 1, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Jan 1, 2025
@MFori MFori requested review from B4nan and drobnikj January 6, 2025 15:17
@MFori MFori marked this pull request as ready for review January 6, 2025 15:17
Copy link
Member

@drobnikj drobnikj left a comment

Choose a reason for hiding this comment

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

Looks fine, I'm not sure about catchNotFoundOrThrow, I would expect that the method throw, why did you use it?

const response = await this.httpClient.call(requestOpts);
return cast(pluckData(response.data));
} catch (err) {
catchNotFoundOrThrow(err as ApifyApiError);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this, should be the same as listItems and return not found error once the dataset does not exist?

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 agree it makes more sense to me too, but I got inspired by DatasetClient.get() which doesn't throw if dataset not exists 🤔 (which I would expect to also throw)

Copy link
Member

Choose a reason for hiding this comment

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

I would also prefer throwing when things are not there, but we should be consistent, and it looks like most methods use the catchNotFoundOrThrow approach.

Copy link
Member

Choose a reason for hiding this comment

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

catchNotFoundOrThrow feels confusing, if you get undefinied, you have no idea if the dataset does not exist or statistics missing.

@drobnikj drobnikj self-requested a review January 10, 2025 15:45
Copy link
Member

@drobnikj drobnikj left a comment

Choose a reason for hiding this comment

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

I would prefere throw over catchNotFoundOrThrow, see discussion. But pre aproving as even catchNotFoundOrThrow makes sence if we want to be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-console Issues with this label are in the ownership of the console team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants