refactor(counter_style): parse int via parse_non_negative #20368
Conversation
|
Heads up! This PR modifies the following files:
|
highfive
added
the
S-awaiting-review
|
|
kwonoj
force-pushed
the
refactor-counter-style
branch
from
e8c8862
to
9790155
Compare
| 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) |
There was a problem hiding this comment.
This is wrong, this needs to be:
let first_symbol_value = input.try(|i| Integer::parse_non_negative(context, i)).ok();instead.
There was a problem hiding this comment.
And first_symbol_value needs to start taking an Integer instead of an i32.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
weight should become an Integer.
| 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)) |
There was a problem hiding this comment.
The first member of Pad should become an Integer, not a u32.
kwonoj
force-pushed
the
refactor-counter-style
branch
from
9790155
to
6675e16
Compare
|
@emilio appreciate for review, tried to update as suggested. Wasn't entirely sure if I could update types to take |
kwonoj
force-pushed
the
refactor-counter-style
branch
from
6675e16
to
3d6a625
Compare
|
*squashing, that is :) |
kwonoj
force-pushed
the
refactor-counter-style
branch
from
3d6a625
to
c8592ea
Compare
|
@emilio sure, rebased to master and squashed into single commit. |
|
@bors-servo r+ Thank you! |
|
📌 Commit c8592ea has been approved by |
highfive
added
S-awaiting-merge
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 -->
|
💔 Test failed - linux-rel-css |
highfive
added
S-tests-failed
|
@bors-servo retry |
|
⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-css... |
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
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 errorsInteger::parseinstead ofexpect_integer#20332 (github issue number if applicable).This change is