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

Remove version-specific prefixes from builder methods #3085

Closed
seanmonstar opened this issue Dec 15, 2022 · 5 comments · Fixed by #3101
Closed

Remove version-specific prefixes from builder methods #3085

seanmonstar opened this issue Dec 15, 2022 · 5 comments · Fixed by #3101
Labels
A-client Area: client. A-http1 Area: HTTP/1 specific. A-http2 Area: HTTP/2 specific. A-server Area: server. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Milestone

Comments

@seanmonstar
Copy link
Member

With the split of the builders into per-version kinds (see #2842 and #2851), I feel there's a new question around whether we should remove the prefixes from the method names. They used to exist because the builders were combined over multiple HTTP versions, so the options were scoped to a version. But now:

mod http1 {
    impl Builder {
        pub fn http1_title_case_headers() {
            // ...
        }
    }
}

It seems all those can have the http1_ prefix removed, and same for the http2::Builders.

Noticed in hyperium/hyper-util#11 (comment).

@seanmonstar seanmonstar added C-feature Category: feature. This is adding a new feature. B-breaking-change Blocked: this is an "API breaking change". B-rfc Blocked: More comments would be useful in determine next steps. labels Dec 15, 2022
@seanmonstar
Copy link
Member Author

Leaving for comments for a few days.

@LucioFranco
Copy link
Member

👍 Would it make sense to rename the builders Http1Builder, etc?

@seanmonstar
Copy link
Member Author

So, http1::Http1Builder? Seems contrary RFC 356. Users who want it can rename it on import: use hyper::client::conn::http1::Builder as Http1Builder.

@LucioFranco
Copy link
Member

Oh didn't even know that existed! Yeah seems fine to me.

@bossmc
Copy link
Contributor

bossmc commented Dec 15, 2022

The APIs are already inconsistent today (e.g. there's http1_title_case_headers but also max_buffer_size on http1::Builder).

Agree with this proposal, the prefixing is now an indirect stutter-naming (conn::http1::Builder::http1_...).

@seanmonstar seanmonstar changed the title Should builder methods remove their prefixes? Remove version-specific prefixes from builder methods Dec 23, 2022
@seanmonstar seanmonstar added A-server Area: server. A-client Area: client. E-easy Effort: easy. A task that would be a great starting point for a new contributor. A-http1 Area: HTTP/1 specific. A-http2 Area: HTTP/2 specific. and removed B-rfc Blocked: More comments would be useful in determine next steps. labels Dec 23, 2022
@seanmonstar seanmonstar added this to the 1.0 RC2 milestone Dec 23, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 24, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 26, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 26, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Michael-J-Ward added a commit to Michael-J-Ward/hyper that referenced this issue Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: client. A-http1 Area: HTTP/1 specific. A-http2 Area: HTTP/2 specific. A-server Area: server. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants