Skip to content

Port ZenHub REST API calls to Java#10

Merged
philippemin merged 50 commits intodevelopfrom
18012_port-zenhub-rest-api-calls-to-java
Feb 12, 2025
Merged

Port ZenHub REST API calls to Java#10
philippemin merged 50 commits intodevelopfrom
18012_port-zenhub-rest-api-calls-to-java

Conversation

@philippemin
Copy link
Copy Markdown
Contributor

@philippemin philippemin commented Dec 17, 2024

Comment on lines +6 to +8
estimate {
value
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're gonna need this when we use getPipelines and getIssuesByPipeline to replace getBoard

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apollo3 has a less "manual" way to handle pagination. If we wanted to use it, from what I understand, we wouldn't be able to make the client return com.ziro.engineering.zenhub.graphql.sdk objects; we would need to map those objects to our own models. Don't think the juice is worth the squeeze for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we will need a chore to move the mappers to github-zenhub-sdk. It would remove a lot of the repetitive methods we're currently writing in our existing mappers. And of course it seems like it would unlock easy pagination for us.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +24 to +29
/** Required by the generated SDK to handle JSON? i.e. nullable JSON */
companion object {
fun nullable(): NullableAdapter<String> {
return NullableAdapter(wrappedAdapter = JsonAdapter())
}
}
Copy link
Copy Markdown
Contributor Author

@philippemin philippemin Feb 6, 2025

Choose a reason for hiding this comment

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

SDK was generating something along the lines of:

jsonAdapter().nullable().fromJson(...), where nullable() returns an Adapter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was just simpler to make the client map the query results to this new data class

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was just simpler to make the client map the query results to this new data class

@justin-sadakhom justin-sadakhom self-requested a review February 7, 2025 14:06
@justin-sadakhom justin-sadakhom self-assigned this Feb 7, 2025
Copy link
Copy Markdown
Member

@justin-sadakhom justin-sadakhom left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some minor feedback in a few places. You'll need to make some small changes because of feedback I left in the ziro-cli PR.

Comment thread build.gradle.kts Outdated

group = "com.ziro.engineering"
version = "2.0.1"
version = "2.0.2"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this shouldn't be 3.0.0? there's definitely some breaking changes to the queries

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we will need a chore to move the mappers to github-zenhub-sdk. It would remove a lot of the repetitive methods we're currently writing in our existing mappers. And of course it seems like it would unlock easy pagination for us.

@philippemin philippemin merged commit 1379699 into develop Feb 12, 2025
@philippemin philippemin deleted the 18012_port-zenhub-rest-api-calls-to-java branch February 12, 2025 16:22
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