Allow cli prefs to have numerical value #14863

Merged
merged 1 commit into from Jan 5, 2017

Projects

None yet

5 participants

@charlesvdv
Contributor
charlesvdv commented Jan 5, 2017 edited

I'm not sure as I'm new with servo but shouldn't the new function parse_opt_prefs be in prefs.rs instead of opts.rs ?


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14842
  • There are tests for these changes OR

This change is Reviewable

@Ms2ger Ms2ger was assigned by highfive Jan 5, 2017
@highfive
highfive commented Jan 5, 2017

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

@Ms2ger
Contributor
Ms2ger commented Jan 5, 2017

r? @jdm

@highfive highfive assigned jdm and unassigned Ms2ger Jan 5, 2017
@jdm

Nice tests! I only have a couple of suggestions.

components/config/opts.rs
@@ -946,6 +939,22 @@ pub fn set_defaults(opts: Opts) {
}
}
+pub fn parse_opt_prefs(pref: &str) {
@jdm
jdm Jan 5, 2017 Member

Let's call this parse_pref_from_command_line instead.

components/config/opts.rs
+ match value {
+ Some(&"false") => PREFS.set(pref_name, PrefValue::Boolean(false)),
+ Some(&"true") | None => PREFS.set(pref_name, PrefValue::Boolean(true)),
+ _ => {
@jdm
jdm Jan 5, 2017 Member

Let's use

Some(value) => match value.parse::<f64>() {
    ...
},
@charlesvdv charlesvdv Allow cli prefs to have numerical value
Fix for #14842.

Extract the code inside a function to unit-test it.
6a22174
@charlesvdv
Contributor

@jdm this should be OK

If you have another idea of work to do for me don't hesitate!

@jdm
Member
jdm commented Jan 5, 2017
@bors-servo
Contributor

📌 Commit 6a22174 has been approved by jdm

@bors-servo
Contributor

⌛️ Testing commit 6a22174 with merge 42e28bf...

@bors-servo bors-servo added a commit that referenced this pull request Jan 5, 2017
@bors-servo bors-servo Auto merge of #14863 - charlesvdv:prefs, r=jdm
Allow cli prefs to have numerical value

I'm not sure as I'm new with servo but shouldn't the new function ```parse_opt_prefs``` be in ```prefs.rs``` instead of ```opts.rs``` ?

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14842

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/14863)
<!-- Reviewable:end -->
42e28bf
@bors-servo bors-servo merged commit 6a22174 into servo:master Jan 5, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@charlesvdv charlesvdv deleted the charlesvdv:prefs branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment