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.

Sign up for GitHub

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

Jump to bottom

feat: add runs() and builds() top level endpoints #468

Merged
merged 4 commits into from
Feb 16, 2024
Merged

feat: add runs() and builds() top level endpoints #468

merged 4 commits into from
Feb 16, 2024

Conversation

foxt451
Copy link
Collaborator

@foxt451 foxt451 commented Dec 6, 2023

Closes #296

@foxt451 foxt451 requested a review from mnmkng December 6, 2023 15:45
@mtrunkat mtrunkat added the t-tooling Issues with this label are in the ownership of the tooling team. label Dec 11, 2023
@drobnikj drobnikj requested review from B4nan and drobnikj and removed request for mnmkng February 5, 2024 12:29
@B4nan
Copy link
Member

B4nan commented Feb 5, 2024 •
edited
Loading

@drobnikj drobnikj (Jakub Drobník) requested review from B4nan and drobnikj

This is not for our team to evaluate, I don't know the story behind it, nor the state of our API when it comes to supporting this.

(codewise it's fine from me)

@B4nan B4nan removed their request for review February 5, 2024 12:39
@drobnikj drobnikj added t-platform Issues with this label are in the ownership of the platform team. and removed t-tooling Issues with this label are in the ownership of the tooling team. labels Feb 5, 2024
drobnikj
drobnikj requested changes Feb 5, 2024
View reviewed changes
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.

Thanks for pushing this forward.
It looks good, can you please add unit tests for new methods?
You can check here for example to have idea how to do it.

src/apify_client.ts
// builds() {
// return new BuildCollectionClient(this._options());
// }
builds(): BuildCollectionClient {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a reference to API docs into comments, the same as we had in the rest of the endpoints.

src/apify_client.ts
// runs() {
// return new RunCollectionClient(this._options());
// }
runs(): RunCollectionClient {
Copy link
Member

Choose a reason for hiding this comment

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

Same as the build list, please add a reference to docs.

@drobnikj
Copy link
Member

drobnikj commented Feb 5, 2024 •
edited
Loading

This is not for our team to evaluate

I added @B4nan because issue #296 solving in this PR is up to your team.

@foxt451 foxt451 requested a review from drobnikj February 6, 2024 13:26
drobnikj
drobnikj approved these changes Feb 8, 2024
View reviewed changes
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.

👍

@B4nan
Copy link
Member

B4nan commented Feb 8, 2024

FYI I did some dependency updates including the shared eslint config, which now enforces import order, that's the reason for the conflicts. Those problems are autofixable.

@B4nan
Copy link
Member

B4nan commented Feb 14, 2024

I'd like to ship new minor version, would be good to include this there, @foxt451 can you please rebase and fix the linting issues?

@B4nan B4nan force-pushed the feat/apify-client-builds-runs branch from 017bf26 to e41dbe7 Compare February 16, 2024 09:11
@B4nan B4nan changed the title feat: enable ApifyClient.builds() and runs() feat: add runs() and builds() top level endpoints Feb 16, 2024
@B4nan B4nan merged commit 252d2ac into master Feb 16, 2024
6 checks passed
@B4nan B4nan deleted the feat/apify-client-builds-runs branch February 16, 2024 09:17
@drobnikj drobnikj added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drobnikj drobnikj drobnikj approved these changes

Assignees
No one assigned
Labels
t-platform Issues with this label are in the ownership of the platform team. validated Issues that are resolved and their solutions fulfill the acceptance criteria.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add runs() and builds() top level endpoints
4 participants
@foxt451 @B4nan @drobnikj @mtrunkat