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

--all-features and --no-default-features #152

Merged
merged 3 commits into from
Jun 30, 2021

Conversation

nagisa
Copy link
Contributor

@nagisa nagisa commented Mar 15, 2021

This adjusts the code and documentation for --all-features and
--no-default-features to work correctly. With --no-default-features
no DefaultAuthenticator is made available. Users are in control of
picking the Connector they want to use, and are not forced to stomach
a dependency on rustls or hyper-tls if their TLS implementation of
choice doesn't happen to match one of the two.

To indicate this, the unstable doc_cfg feature is used to build
documentation on docs.rs. That way the generated documentation has
notices on these types that look as such:

This is supported on crate features hyper-rustls or hyper-tls only.

Additionally this functionality is tested via additional coverage in the
Actions' CI.

This, I believe, should be a backwards compatible change, with a very small
exception – users who have RUSTDOCFLAGS='--cfg=yup_oauth2_docsrs' set
and are building with a non-nightly compiler will observe failures to generate
documentation.

@nagisa
Copy link
Contributor Author

nagisa commented Mar 15, 2021

Some screenshots of how this is represented in the documentation:

Documentation page listing items available within the module and showing the hyper-rustls or hyper-tls in a blue background near those that are supported conditionally
Documentation page of the DefaultAuthenticator item, with a "This is supported on crate features hyper-rustls or hyper-tls only" notice in it

@dermesser
Copy link
Owner

Thank you, this change looks very useful! Can you check the Github Action that failed above? Is it intended, or did something go wrong?

@nagisa
Copy link
Contributor Author

nagisa commented Mar 16, 2021

Yeah, this is not exactly complete yet, but should be sooner rather than later. I think we will also want to add some API surface to builders in a followup to allow people to construct various flows with their connector (as the currently present builder entry points are not available without DefaultHyperClient)

@nagisa nagisa force-pushed the nagisa/features branch 2 times, most recently from be7d072 to f081b9c Compare March 19, 2021 21:09
Copy link
Owner

@dermesser dermesser left a comment

Choose a reason for hiding this comment

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

Some screenshots of how this is represented in the documentation:

this looks good

src/authenticator.rs Show resolved Hide resolved
@dermesser
Copy link
Owner

dermesser commented Jun 28, 2021

I think we will also want to add some API surface to builders in a followup to allow people to construct various flows with their connector (as the currently present builder entry points are not available without DefaultHyperClient)

just to clarify - would you be ready to add these methods? Or are you in favor of first merging this PR and then figuring out the best way forward? (I don't expect you to add the methods in this PR, we don't need big PRs -- just wanted to clarify)

@nagisa
Copy link
Contributor Author

nagisa commented Jun 28, 2021

I think its a reasonable time to consider that given that my other PR is also a breaking change (#158) and that gives more leeway to go for a no-compromise API.

This adjusts the code and documentation for `--all-features` and
`--no-default-features` to work correctly. With `--no-default-features`
no `DefaultAuthenticator` is made available. Users are in control of
picking the `Connector` they want to use, and are not forced to stomach
a dependency on `rustls` or `hyper-tls` if their TLS implementation of
choice doesn't happen to match one of the two.

To indicate this, the unstable `doc_cfg` feature is used to build
documentation on docs.rs. That way the generated documentation has
notices on these types that look as such:

> This is supported on crate features hyper-rustls or hyper-tls only.

Additionally this functionality is tested via additional coverage in the
Actions' CI.
@nagisa
Copy link
Contributor Author

nagisa commented Jun 29, 2021

This should be ready now.

//!
//! It is also a better use of resources (memory, sockets, etc.)

async fn r#use<C>(
Copy link
Owner

Choose a reason for hiding this comment

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

is r#use some new syntax I'm not aware of, or did you choose this name intentionally? It reminds me of the raw string literal syntax

Copy link
Contributor Author

@nagisa nagisa Jun 29, 2021

Choose a reason for hiding this comment

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

It is a raw identifier, and allows using words as identifiers that are otherwise reserved, such as use. I had a hard time coming up with another name here. Open to suggestions.

@dermesser
Copy link
Owner

Great work, this looks very good!

@dermesser dermesser merged commit ac0e5f6 into dermesser:master Jun 30, 2021
@dermesser
Copy link
Owner

Would you prefer to make this a new release (new major version) right away, or should we wait for other opinions on how this API works?

@nagisa nagisa deleted the nagisa/features branch June 30, 2021 09:52
@nagisa
Copy link
Contributor Author

nagisa commented Jun 30, 2021

I don't think this change warrants a release I think it is fine to hold off on releasing before e.g. #158 or something similar makes it in.

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.

None yet

2 participants