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

Add associated pull requests and commit compare functionality #413

Merged

Conversation

matthewgapp
Copy link
Contributor

@matthewgapp matthewgapp commented Jul 16, 2023

Copy link
Contributor Author

@matthewgapp matthewgapp Jul 16, 2023

Choose a reason for hiding this comment

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

These types were created automatically using the json schema here

The types were generated using https://quicktype.io/

pub patch: Option<String>,
pub previous_filename: Option<String>,
// unlike the schema online, this can be null
pub raw_url: Option<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

discovered this while using the endpoints

}

/// Sends the actual request.
pub async fn send(self) -> crate::Result<models::commits::CommitComparison> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this endpoint accepts pagination params but doesn't return an array so I didn't wrap the return type in Page

@matthewgapp matthewgapp marked this pull request as ready for review July 16, 2023 03:39
@@ -29,7 +29,7 @@ async fn main() -> octocrab::Result<()> {
println!(
"{id} | {url} | [{files}] | {description}",
id = gist.id,
url = gist.html_url.to_string(),
url = gist.html_url,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy --fix

@@ -19,6 +19,6 @@ async fn main() -> octocrab::Result<()> {
.public(false)
.send()
.await?;
println!("Done, created: {url}", url = gist.html_url.to_string());
println!("Done, created: {url}", url = gist.html_url);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy --fix

@@ -18,13 +18,13 @@ async fn setup_api(template: ResponseTemplate) -> MockServer {
let mock_server = MockServer::start().await;

Mock::given(method("GET"))
.and(path(format!("/user/memberships/orgs")))
.and(path("/user/memberships/orgs".to_string()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy --fix

.respond_with(template)
.mount(&mock_server)
.await;
setup_error_handler(
&mock_server,
&format!("GET on /user/membership/orgs was not received"),
&"GET on /user/membership/orgs was not received".to_string(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy --fix

@@ -20,6 +16,7 @@ pub struct WorkflowRunEventPayload {
#[non_exhaustive]
pub enum WorkflowRunEventAction {
Requested,
InProgress,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flyby fix

@XAMPPRocky
Copy link
Owner

Thank you for your PR! Since I merged your other PR, you'll need to rebase these changes to remove the similar changes. In the future I would recommend making the things like clippy --fix a separate PR, as then I could merge all three PRs at once.

@maflcko
Copy link
Sponsor Contributor

maflcko commented Jul 18, 2023

I think the "Squash and merge" strategy will take care of dropping the duplicate changes.

@XAMPPRocky
Copy link
Owner

I think the "Squash and merge" strategy will take care of dropping the duplicate changes.

To be clear, it's also about review, it's harder for me to review a PR if the same changes exist.

@maflcko
Copy link
Sponsor Contributor

maflcko commented Jul 18, 2023

Ok, now it needs a merge or rebase either way :)

@matthewgapp matthewgapp force-pushed the matt/improve/add-commit-compare branch from 2477b6b to dea2ea2 Compare July 27, 2023 02:14
@matthewgapp
Copy link
Contributor Author

matthewgapp commented Jul 27, 2023

Thanks for the review and apologies for the delay @XAMPPRocky and @MarcoFalke. Rebased.

@XAMPPRocky
Copy link
Owner

@matthewgapp You need to update your tests to #[tokio::test], they're currently failing for not being in a Tokyo runtime.

@matthewgapp
Copy link
Contributor Author

@matthewgapp You need to update your tests to #[tokio::test], they're currently failing for not being in a Tokyo runtime.

Apologies. I should have ran cargo test. I didn't see any signs that crate::Octocrab::default() needed an async runtime but I guess internally it spawns a tokio task when it creates a tower middleware buffer service.

@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 6e91b68 into XAMPPRocky:main Jul 28, 2023
10 checks passed
@github-actions github-actions bot mentioned this pull request Jul 28, 2023
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.

3 participants