-
Notifications
You must be signed in to change notification settings - Fork 32
Use a builder to configure the formatter #113
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #113 +/- ##
==========================================
- Coverage 99.27% 99.25% -0.03%
==========================================
Files 6 6
Lines 3887 3870 -17
Branches 3887 3870 -17
==========================================
- Hits 3859 3841 -18
- Misses 20 21 +1
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
You may look at the tests to see the usage, if you like it I can finish converting the tests.
Formatter::config().params(params).inline(true)... .format() |
|
Do any of the examples in the readme need to be updated as well? |
|
All of them, thank you for reminding me :) We should discuss a little bit few details btw |
| /// | ||
| /// Default: None | ||
| pub ignore_case_convert: Option<Vec<&'a str>>, | ||
| ignore_case_convert: Option<Vec<&'a str>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be later changed to accept AsRef<[&str]> so one can pass to the builder also &[&str],[&str; N] and so on.
| Named(Vec<(String, String)>), | ||
| Indexed(Vec<String>), | ||
| pub enum QueryParams<'a> { | ||
| Named(Cow<'a, [(String, String)]>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be made simpler by taking only by reference, but I wanted your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think having either either all owned or all references would make usage simpler, and all references is likely the better option due to performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I did now, it is almost transparent to the user.
No description provided.