Conversation
Pull Request Test Coverage Report for Build 18113147319Details
💛 - Coveralls |
payjoin-ffi/src/request.rs
Outdated
| /// | ||
| /// This is full URL with scheme etc - you can pass it right to `reqwest` or a similar library. | ||
| pub url: Arc<Url>, | ||
| pub url: String, |
There was a problem hiding this comment.
I have a feeling this shouldn't be pub any longer either. I also wonder what use it has as a Record if it's now really an Object where we just need those functions as getters.
There was a problem hiding this comment.
Good point, I had run up against the wall at first but didn;t consider that any other ffi records would need to call this as an Arc for that to work, fixed
payjoin/src/core/request.rs
Outdated
|
|
||
| pub fn url(&self) -> String { self.url.to_string() } | ||
|
|
||
| pub fn content_type(&self) -> String { self.content_type.to_string() } | ||
|
|
||
| pub fn body(&self) -> Vec<u8> { self.body.clone() } |
There was a problem hiding this comment.
I think we typically default to borrowing with getters. Did you choose to clone for convenience?
|
what is the motivation for this? isn't it simpler to just change the field types themselves instead of adding accessors? all of the fields must be read for a valid request to be constructed, returning them all at once allows for destructuring in match, so afaict accessors only take away ergonomics, and any logic that is done in the accessors could be done before constructing this |
benalleng
force-pushed
the
remove-request-pub-fields
branch
from
7ad1160 to
e0a3565
Compare
The url field being the critical one here, by not changing the type of the field and instead using accessors the intention was to prevent having to do any logic transforming the field to and from a String and a Url. Keeping the Url out of the public API with minimal logic changes in the Request parsing |
benalleng
force-pushed
the
remove-request-pub-fields
branch
2 times, most recently
from
9bf44c0 to
761a1c1
Compare
This commit reduces the visibility of all the Request fields to private and replaces the access with public getter methods that returns the fields as refs.
benalleng
force-pushed
the
remove-request-pub-fields
branch
from
761a1c1 to
bf1874d
Compare
To make the fields read only and keep the type safety of |
|
That said making Request url |
|
Closed as uneeded following #1122 |
4 participants
These three commits reduce the visibility of the Request Struct's internal fields and replaces their external access with a few getter methods.
I opted for
url()instead ofurl_as_str()as we are not exposing a url to the public API anywhere so there does not need to be a distinction as all Urls in our public API are just stringsCloses #513
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.