-
Notifications
You must be signed in to change notification settings - Fork 366
Refactor length parsing and make offset a length #1330
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
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1330 +/- ##
==========================================
+ Coverage 65.71% 65.95% +0.23%
==========================================
Files 50 50
Lines 8041 8209 +168
Branches 925 962 +37
==========================================
+ Hits 5284 5414 +130
- Misses 2757 2795 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Looks fine to me. Don't have time to review the whole thing though |
|
Can I go ahead? |
|
I haven't looked into the code in detail yet, but treating offset as a length of (min, max) seems odd as the values stored therein have completely different meaning, though they have the same structure. What is puzzling me is that Maybe I am missing the bigger picture here. |
My idea was making in a future refactor of the parser a unified "tuple"-like type. Also it is odd for me that we have the syntax for couple of values but offset is the only one using 1x2. For the height I wanted to add a min and max because dynamic heights was one of the low hanging fruit I am gonna add soon (aka one of the next prs) |
Move sanitization from the length parser to the settings file. Also make offset a length with the (N,N) syntax.
offset still accepts the old syntax for backward compatibility.
This will lay the foundations for implementing #991 and dynamic height.
This is a partial reversal of commit acf0a0f, @fwsmit what do you think?