Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uprefactor(counter_style): parse int via parse_non_negative #20368
Conversation
highfive
commented
Mar 20, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Mar 20, 2018
|
|
|
Thanks for working on this! |
| try_match_ident_ignore_ascii_case! { input, | ||
| "cyclic" => Ok(System::Cyclic), | ||
| "numeric" => Ok(System::Numeric), | ||
| "alphabetic" => Ok(System::Alphabetic), | ||
| "symbolic" => Ok(System::Symbolic), | ||
| "additive" => Ok(System::Additive), | ||
| "fixed" => { | ||
| let first_symbol_value = input.try(|i| i.expect_integer()).ok(); | ||
| let first_symbol_value = Integer::parse_non_negative(context, input) |
This comment has been minimized.
This comment has been minimized.
emilio
Mar 21, 2018
Member
This is wrong, this needs to be:
let first_symbol_value = input.try(|i| Integer::parse_non_negative(context, i)).ok();instead.
This comment has been minimized.
This comment has been minimized.
emilio
Mar 21, 2018
Member
And first_symbol_value needs to start taking an Integer instead of an i32.
This comment has been minimized.
This comment has been minimized.
emilio
Mar 21, 2018
Member
Err, and this doesn't really need to be non-negative, so should be:
let first_symbol_value = input.try(|i| Integer::parse(context, i)).ok();| let symbol = symbol.or_else(|_| Symbol::parse(context, input))?; | ||
| Ok(AdditiveTuple { | ||
| weight: weight as u32, | ||
| weight: weight.value() as u32, |
This comment has been minimized.
This comment has been minimized.
| let pad_with = pad_with.or_else(|_| Symbol::parse(context, input))?; | ||
| Ok(Pad(min_length as u32, pad_with)) | ||
| Ok(Pad(min_length.value() as u32, pad_with)) |
This comment has been minimized.
This comment has been minimized.
|
Err, I meant to request changes above :) |
|
@emilio appreciate for review, tried to update as suggested. Wasn't entirely sure if I could update types to take |
|
Looks great! Mind sqashing the commits? |
|
*squashing, that is :) |
|
@emilio sure, rebased to master and squashed into single commit. |
|
@bors-servo r+ Thank you! |
|
|
refactor(counter_style): parse int via parse_non_negative <!-- Please describe your changes on the following line: --> Relates to #20332. This PR intends to refactor `counter_style` to use `parse_non_negative` where applicable, mostly which uses `expect_integer` currently. Change still grabs value from parsed result then assign it into each struct as needed and does not change definition of struct (i.e `AdditiveTuple`) itself. --- <!-- 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 #20332 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ - This PR has locally built & ran unit test on macOS. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/20368) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
|
|
|
kwonoj commentedMar 20, 2018
•
edited by SimonSapin
Relates to #20332.
This PR intends to refactor
counter_styleto useparse_non_negativewhere applicable, mostly which usesexpect_integercurrently. Change still grabs value from parsed result then assign it into each struct as needed and does not change definition of struct (i.eAdditiveTuple) itself../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is