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 methods to builder for changing timeouts #498

Merged

Conversation

johnrichardrinehart
Copy link
Contributor

Work related to #497 .

Copy link
Owner

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! LGTM I have just one minor change request.

src/lib.rs Outdated
@@ -485,6 +485,27 @@ impl OctocrabBuilder<NoSvc, DefaultOctocrabBuilderConfig, NoAuth, NotLayerReady>
self
}

/// Set the connect timeout.
#[cfg(feature = "timeout")]
Copy link
Owner

Choose a reason for hiding this comment

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

Does this really need its own feature flag? I think it's probably better to just have it included by default.

Copy link
Contributor Author

@johnrichardrinehart johnrichardrinehart Dec 1, 2023

Choose a reason for hiding this comment

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

I don't know. But, much of the timeout functionality is already guarded behind the timeout feature flag. I figured it'd be good to keep it in order to keep things consistent and to clean up the compiled output.

Note that I don't know what value it would provide users. If this code isn't guarded behind the timeout flag since then users who are not using the timeout flag will see methods that won't be of any use to them.

@XAMPPRocky
Copy link
Owner

Thank you for your PR, and congrats on your first contribution! 🎉

@XAMPPRocky XAMPPRocky merged commit ee49f5c into XAMPPRocky:main Jan 15, 2024
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
@CommanderStorm
Copy link
Contributor

CommanderStorm commented Jan 15, 2024

@XAMPPRocky I think the feature flag mentioned above is indeed required:
image

I think reverting f1c94c1 does fix this issue.

This was referenced Jan 15, 2024
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