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.

Sign up for GitHub

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

Jump to bottom

Allow setting numeric preferences from the command line #14842

Closed
jdm opened this issue Jan 4, 2017 · 3 comments · Fixed by #14863
Closed

Allow setting numeric preferences from the command line #14842

jdm opened this issue Jan 4, 2017 · 3 comments · Fixed by #14863
Labels
A-platform/utils I-papercut Small but painful.

Comments

@jdm
Copy link
Member

jdm commented Jan 4, 2017

The code to process --pref name=value command line arguments currently recognizes boolean values as special, and otherwise defaults to treating the value as a string. We should check if the value can be parsed as a number and set a numeric preference if possible.

Code: components/config/opts.rs

@charlesvdv Want to fix this?
@jdm jdm added A-platform/utils I-papercut Small but painful. labels Jan 4, 2017
@jdm
Copy link
Member Author

jdm commented Jan 4, 2017 •
edited

Also, it might be useful to make it possible to write automated tests for our command line argument processing code. At minimum the preference-parsing code could be extracted to a helper function and we could add a unit test to tests/unit/servo_config/opts.rs.

@asajeffrey
Copy link
Member

This came up in the context of #14312 which is using an opt rather than a pref for this reason.

@charlesvdv
Copy link
Contributor

I'm interested ! Thanks @jdm !

charlesvdv added a commit to charlesvdv/servo that referenced this issue Jan 5, 2017
@charlesvdv
Allow cli prefs to have numerical value
91a8fb2 
Fix for servo#14842.

Extract the code inside a function to unit-test it.
@charlesvdv charlesvdv mentioned this issue Jan 5, 2017
Merged
4 tasks
charlesvdv added a commit to charlesvdv/servo that referenced this issue Jan 5, 2017
@charlesvdv
Allow cli prefs to have numerical value
48bcd1a 
Fix for servo#14842.

Extract the code inside a function to unit-test it.
charlesvdv added a commit to charlesvdv/servo that referenced this issue Jan 5, 2017
@charlesvdv
Allow cli prefs to have numerical value
6a22174 
Fix for servo#14842.

Extract the code inside a function to unit-test it.
bors-servo pushed a commit that referenced this issue Jan 5, 2017
Auto merge of #14863 - charlesvdv:prefs, r=jdm
42e28bf 
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-platform/utils I-papercut Small but painful.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow cli prefs to have numerical value charlesvdv/servo
3 participants
@jdm @asajeffrey @charlesvdv