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

Refactoring RequestInit to use a Builder Pattern #22521

Merged

Conversation

Projects
None yet
8 participants
@lucasfantacuci
Copy link
Contributor

commented Dec 21, 2018

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22427
  • These changes do not require tests because it is a code refactoring.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Dec 21, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Dec 21, 2018

Heads up! This PR modifies the following files:

  • @KiChjang: components/net_traits/request.rs
@lucasfantacuci

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

I implemented the builder pattern as a non-consuming builder. ref: https://doc.rust-lang.org/1.0.0/style/ownership/builders.html

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:use_build_pattern_with_requestinit branch from 579725c to 7a5ef1b Dec 21, 2018

@lucasfantacuci lucasfantacuci changed the title WIP: Use builder pattern with RequestInit Refactoring RequestInit to use a Builder Pattern Dec 21, 2018

@jdm
Copy link
Member

left a comment

Sorry about the delay! Thanks for making these changes; they have made clear some interesting holes in our current network requests that I plan to file issues about!

Show resolved Hide resolved components/net_traits/request.rs Outdated
cache_mode: request.cache_mode,
..NetTraitsRequestInit::default()
}
NetTraitsRequestInit::new(request.url())

This comment has been minimized.

Copy link
@jdm

jdm Jan 15, 2019

Member

I would actually like to revert the change to this function, and remove the use of ..NetTraitsRequesInit::default(). That will make the compiler catch when we add members to NetTraitsRequestInit and need to be sure they are initialized correctly.

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 4, 2019

Author Contributor

Sorry, I didn't get you.

This comment has been minimized.

Copy link
@jdm

jdm Apr 4, 2019

Member

I would like to revert this particular change: this method should construct a new NetTraitsRequestInit object with an explicit structvliteral that initializes every field (instead of using ..NetTraitsRequestInit::default()).

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 4, 2019

Author Contributor

What do you think about a method overload to the method to the Builder new passing all the arguments or the Request Object?

I'm just thinking about the separation of concerns.

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 4, 2019

Author Contributor

What do you think now?

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 5, 2019

Author Contributor

Then we will be able to construct this object without setting the properties directly by removing the "pub" from them.

e.g.:

RequestBuilder {
    url: url
    ...
}

It will throw an error if you are building outside of his scope.

This comment has been minimized.

Copy link
@jdm

jdm Apr 8, 2019

Member

The advantage of allowing some code like request_init_from_request to create the RequestInit value directly is that it's easier to understand code like use_url_credentials: false instead of a false value that is one of 20 arguments. I think it's fine to keep the field values of RequestInit public and build it directly, rather than adding a method.

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 8, 2019

Author Contributor

yeah, i've got you.

@jdm

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

error: unused import: `std::default::Default`
  --> components/net_traits/request.rs:11:5
   |
11 | use std::default::Default;
   |     ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
@lucasfantacuci

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

My apologies, I was away. I gonna retake this task and finish it.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

☔️ The latest upstream changes (presumably #23103) made this pull request unmergeable. Please resolve the merge conflicts.

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:use_build_pattern_with_requestinit branch from 7a5ef1b to 1bb9073 Apr 3, 2019

@lucasfantacuci lucasfantacuci changed the title Refactoring RequestInit to use a Builder Pattern [WIP]Refactoring RequestInit to use a Builder Pattern Apr 4, 2019

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:use_build_pattern_with_requestinit branch 5 times, most recently from f3b6ef9 to 2712351 Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@lucasfantacuci: 🔑 Insufficient privileges: not in try users

1 similar comment
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@lucasfantacuci: 🔑 Insufficient privileges: not in try users

bors-servo added a commit that referenced this pull request Apr 10, 2019

Auto merge of #22521 - lucasfantacuci:use_build_pattern_with_requesti…
…nit, r=jdm

Refactoring RequestInit to use a Builder Pattern

<!-- Please describe your changes on the following line: -->

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22427
- [x] These changes do not require tests because it is a code refactoring.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

⌛️ Testing commit c3803b0 with merge 5370151...

bors-servo added a commit that referenced this pull request Apr 10, 2019

Auto merge of #22521 - lucasfantacuci:use_build_pattern_with_requesti…
…nit, r=jdm

Refactoring RequestInit to use a Builder Pattern

<!-- Please describe your changes on the following line: -->

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22427
- [x] These changes do not require tests because it is a code refactoring.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)
<!-- Reviewable:end -->
@@ -24,7 +24,7 @@ use crate::task_source::TaskSourceName;
use ipc_channel::ipc;
use ipc_channel::router::ROUTER;
use js::jsapi::JSAutoCompartment;
use net_traits::request::RequestInit as NetTraitsRequestInit;
use net_traits::request::RequestBuilder as NetTraitsRequestInit;

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 10, 2019

Member

This probably doesn't need the renaming anymore.

use_cors_preflight: request.use_cors_preflight,
credentials_mode: request.credentials_mode,
use_url_credentials: request.use_url_credentials,
use_url_credentials: false,

This comment has been minimized.

Copy link
@KiChjang

KiChjang Apr 10, 2019

Member

Why is this changed to false?

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 10, 2019

Author Contributor

It may be a lack of attention.

This comment has been minimized.

Copy link
@lucasfantacuci

lucasfantacuci Apr 10, 2019

Author Contributor

The default value is false.

@lucasfantacuci lucasfantacuci force-pushed the lucasfantacuci:use_build_pattern_with_requestinit branch from c3803b0 to 6b2be9b Apr 10, 2019

@KiChjang

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@bors-servo r=jdm,KiChjang

Thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

📌 Commit 6b2be9b has been approved by jdm,KiChjang

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

⌛️ Testing commit 6b2be9b with merge cb5dce5...

bors-servo added a commit that referenced this pull request Apr 10, 2019

Auto merge of #22521 - lucasfantacuci:use_build_pattern_with_requesti…
…nit, r=jdm,KiChjang

Refactoring RequestInit to use a Builder Pattern

<!-- Please describe your changes on the following line: -->

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22427
- [x] These changes do not require tests because it is a code refactoring.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

💥 Test timed out

@KiChjang

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

⌛️ Testing commit 6b2be9b with merge f14d199...

bors-servo added a commit that referenced this pull request Apr 11, 2019

Auto merge of #22521 - lucasfantacuci:use_build_pattern_with_requesti…
…nit, r=jdm,KiChjang

Refactoring RequestInit to use a Builder Pattern

<!-- Please describe your changes on the following line: -->

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22427
- [x] These changes do not require tests because it is a code refactoring.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

⌛️ Testing commit 6b2be9b with merge 9ab0af0...

bors-servo added a commit that referenced this pull request Apr 11, 2019

Auto merge of #22521 - lucasfantacuci:use_build_pattern_with_requesti…
…nit, r=jdm,KiChjang

Refactoring RequestInit to use a Builder Pattern

<!-- Please describe your changes on the following line: -->

If RequestInit::new accepts all of the mandatory arguments and then the builder pattern is used for customizable options, the resulting code might be easier to match against specification text like

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22427
- [x] These changes do not require tests because it is a code refactoring.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22521)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@bors-servo bors-servo merged commit 6b2be9b into servo:master Apr 11, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@lucasfantacuci lucasfantacuci deleted the lucasfantacuci:use_build_pattern_with_requestinit branch Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.