support for parsing arbitrary weight numbers and stretch percentages #11

russaa · 2018-08-10T11:17:14Z

adds support for parsing extended capabilities defined in CSS Fonts Module Level 4 for

font-weight: allow arbitrary numbers from 1 to 1000
- https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight
- https://drafts.csswg.org/css-fonts-4/#valdef-font-weight-number
font-stretch: allow (positive) percentage numbers
https://developer.mozilla.org/en-US/docs/Web/CSS/font-stretch
https://drafts.csswg.org/css-fonts-4/#valdef-font-stretch-percentage

this PR also extends parsing of valid numbers:
namely considering leading + and - when parsing numbers
See also
https://developer.mozilla.org/en-US/docs/Web/CSS/integer
https://drafts.csswg.org/css-values-3/#numeric-types

…tages

codecov · 2018-08-10T11:19:38Z

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #11   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          67    115   +48     
  Branches       18     34   +16     
=====================================
+ Hits           67    115   +48

Impacted Files	Coverage Δ
src/index.ts	`100% <100%> (ø)`	⬆️
src/helpers.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6be37ac...45a1db5. Read the comment docs.

jednano

Thanks for the PR! Looks like we need to create a new project called parse-css-integer that would handle some of the logic in this PR though.

jednano · 2018-08-10T16:27:14Z

src/helpers.ts

+		|| isOnlyNumber(value, true);
+}
+
+function isOnlyNumber(value: string, isPercent: boolean): boolean {


Can we change the 2nd arg to { isPercent = false } = {}? It makes the implementation read better:

isOnlyNumber(value, { isPercent: true });

Also, since false is the default, no need to indicate it:

isOnlyNumber(value);

Also, instead of isPercent here, would it make more sense to name it isPercentAccepted? Or am I misunderstanding something?

jednano · 2018-08-10T16:36:22Z

src/helpers.ts

+}
+
+function isOnlyNumber(value: string, isPercent: boolean): boolean {
+	const match = /^(\+|-)?(\d|\.)/.exec(value);


What happens if provided a solo . or +. w/o any digits to follow? Also, you might want to cache this regex outside of the function for (marginal) performance reasons.

Also, let's exit early below this line with:

if (!match) { return false; } const [sign, value, percent] = match;

This would only work, of course, if the percent were included in the regex like:

const cssNumberPattern = /^(\+|-)?([\d\.]+)(%)?$/;

Which makes me wonder, can't this CSS number parsing be its own package, used outside of parse-css-font? Or is it only ever used in a font?

Also, what is the value of the isPercent flag if we can just detect it in the regex?

const isPercent = !!percent;

I am not sure about this (changing the regular expression):

as to the first question, this regular expression is only a pre-test: any non-number value would get rejected at parseFloat(val).toString() === val

this regular expression is also used for determining if there is leading zero missing for fraction-only values (e.g. .24): in this case a zero has to be prepended, so that the test parseFloat(val).toString() === val works

-> so changing the regular expression would also require to re-implement the test (for checking if the string really only contains a number, or something more, or something else entirely)

Either way, for readability, changing the variable names to sign, value and percent would be much more clear.

jednano · 2018-08-10T16:37:22Z

src/helpers.ts

+
+function isOnlyNumber(value: string, isPercent: boolean): boolean {
+	const match = /^(\+|-)?(\d|\.)/.exec(value);
+	if (match && (!isPercent || /%$/.test(value))) {


Does this mean you're accepting values that end with % when isPercent = false? I'm a bit confused by that.

the meaning of isPercent is something like "only return true if it is number and also has a percent sign", so the naming is probably a bit of a poor choice, maybe onlyPercent would make it more clear:

it means, either accept any numbers or, if isPercent is true, require that a percent sign follows the number

The "and also" part is the confusing part and violates the SRP. Try to make your functions do one and only one thing to clear up any confusion.

jednano · 2018-08-10T16:45:41Z

src/helpers.ts

+	const match = /^(\+|-)?(\d|\.)/.exec(value);
+	if (match && (!isPercent || /%$/.test(value))) {
+		let val = isPercent ? value.substring(0, value.length - 1) : value;
+		if (match[1] === '+') {


If the above were implemented, this would read:

if (sign === '+') {

jednano · 2018-08-10T16:47:20Z

src/helpers.ts

+		}
+		if (match[2] === '.') {
+			if (match[1] === '-') {
+				val = '-0' + val.substring(1);


This is a bit hard to read. I think some refactoring is in order to make it clear what the intention is here.

jednano · 2018-08-10T17:00:17Z

src/index.ts

+		} else if (prelimStretchNum) {
+			font.size = font.stretch;
+			font.stretch = 'normal';
+			font.family = cssListHelpers.splitByCommas(tokens.join(' ')).map(unquote) as string[];


Ditto on the as string[].

jednano · 2018-08-10T17:03:05Z

src/index.ts

@@ -119,6 +146,23 @@ function parseLineHeight(value: string) {
 	const parsed = parseFloat(value);
 	if (parsed.toString() === value) {
 		return parsed;
+	} else {
+		const match = /^(\+|-)?(\.)?/.exec(value) as RegExpMatchArray;


We can probably reuse the above-mentioned regex that I asked you to cache here.

Also, the TypeScript compiler will infer the type, so you shouldn't need as RegExpMatchArray here.

without the cast, the compiler will complain that the match could be null:
but the regular expression will always match, since it has 2 optional parts.

So either have this cast, or add a not-null check to convince the compiler that using match[1] is OK -- I decided to use the cast, but if you prefer the not-null check, I will change it.

Similar for those other casts above:
it's i.m.o. a matter of taste, if you use the cast or add an explicit check -- I prefer the cast for instances where I know that the explicit check is unnecessary, as it does not add any JavaScript code after the compilation

Why are you so sure the value wouldn't be null?

it checks for (a) a beginning "^" and (b) two optional groups, so as long as the string has a beginning it will match

I'm not sure what's wrong w/ your compiler or at what point it's giving you these errors, but I can tell you that the TypeScript playground has no problems with the following code:

function foo(value: string) { const match = /^(foo)?(bar)?/.exec(value); return match[1]; }

If I mouse-over const match it tells me that match is a RegExpExecArray.

Are you seeing something different? Note that RegExpExecArray and RegExpMatchArray are two different interfaces.

no you are right, it should be RegExpExecArray (since this was only for silencing the compiler I did not look closely enough)

still: for exec() my compiler shows me a return value of RegExpExecArray | null, so without the cast there would be an explicit check for non-null

jednano · 2018-08-10T17:04:20Z

src/index.ts

+		const match = /^(\+|-)?(\.)?/.exec(value) as RegExpMatchArray;
+		let val: string = value;
+		if (match[1] === '+') {
+			val = val.substring(1);


This shouldn't be needed once we deconstruct the match into [sign, value, percent].

jednano · 2018-08-10T17:04:46Z

src/index.ts

+			val = val.substring(1);
+		}
+		if (match[2] === '.') {
+				// NOTE although not specifically prohibited, we do not consider negative numbers for line-height valid:


Were all these comments left here on purpose? Were these issues ever resolved?

yes, I left this comment on purpose, although I will happily remove it:

The underlying reason for the comment was that I am not really sure why there is a conversion from string to number at this point.

I did not want to touch the line-height parsing code at all, but the test framework requires the conversion of the string values to number (otherwise the extended number tests could not have been added, or would have needed to exclude tests where line-height was part of the test input).

The comment is wrong in that really negative numbers are not allowed for line-height, but as I said, I did not want to mess around too much with this code as I was not really sure what the intention was here:
why convert to numbers? and why omit the units?

jednano · 2018-08-10T17:05:33Z

src/index.ts

@@ -119,6 +146,23 @@ function parseLineHeight(value: string) {
 	const parsed = parseFloat(value);
 	if (parsed.toString() === value) {
 		return parsed;
+	} else {


Another superfluous else.

This else can still be removed. To be clear, the code inside the else block can be preserved, but the scope of this block can be unindented by one level.

russaa · 2018-08-13T12:40:20Z

I will work on those changes & post an update

for some that were not fully clear to me, I added comments

* extract regular expressions for reusing instances * split parse-as-number-only and parse-as-percent-number-only * decompose regular expression match

russaa · 2018-08-13T20:14:55Z

I think I addressed most of your concerns except for some few where I decided on a slightly different solution:

mainly

in helpers.ts not extracting/using regular expression const cssNumberPattern = /^(\+|-)?([\d\.]+)(%)?$/; etc.
in index.ts I did change parseLineHeight() only slightly: I want to hear your general feedback first

jednano

Looking better. Couple more things and I think I'm going to take a stab at this locally to see if any more improvements can be made.

Any thoughts/opinions about extracting any of this logic into a separate project? Specifically, some of the helper functions?

jednano · 2018-08-13T23:34:09Z

src/index.ts

@@ -119,6 +146,23 @@ function parseLineHeight(value: string) {
 	const parsed = parseFloat(value);
 	if (parsed.toString() === value) {
 		return parsed;
+	} else {


This else can still be removed. To be clear, the code inside the else block can be preserved, but the scope of this block can be unindented by one level.

jednano · 2018-08-13T23:34:54Z

src/index.ts

@@ -119,6 +140,19 @@ function parseLineHeight(value: string) {
 	const parsed = parseFloat(value);
 	if (parsed.toString() === value) {
 		return parsed;
+	} else {
+		const match = /^(\+)?(\.)?/.exec(value) as RegExpExecArray;


Let's add the following to the tsconfig.json compiler options:

"downlevelIteration": true,

And then you can remove this as RegExpExecArray; here.

…ssary cast

russaa · 2018-08-14T12:21:52Z

I made the suggested changes.

As for a separate number-parsing project:
I feel I do not have enough overview to have an informed opinion about this -- I don't know where it would/could be used (except for here of course), so I do not really know if it would be worth extracting (and extending) the number-parsing.

If a separate module, it should be able to parse/handle integer/number and probably also, at least to some extent, handle number-unit combinations such as length, percentage, frequency, angle, time, resolution.

jednano

I'm very concerned by the amount of code that this PR is introducing, so my comments are meant to reduce as much code as possible and defer specific parsing tasks to other packages.

jednano · 2018-08-14T23:02:30Z

src/index.ts

@@ -120,5 +141,17 @@ function parseLineHeight(value: string) {
 	if (parsed.toString() === value) {
 		return parsed;
 	}
+	const match = /^(\+)?(\.)?/.exec(value);


OK the more I look at this logic, the more I think installing parse-css-dimension might be useful in replacing this logic. What do you think?

No broken tests with the following implementation:

function parseLineHeight(value: string) { try { const parsed = parseCssDimension(value); if (parsed.type === 'number') { return parsed.value; } } catch (err) { // noop } return value; }

yes that would probably be better -- ideally the parsing should also be extended to reject negative values for line height (and invalid dimensions/units)

I see what you mean. Looks like there's a validation aspect that needs to be employed here. I just tried this:

.foo { line-height: -1; }

on http://jigsaw.w3.org/css-validator/#validate_by_input and got the following error:

Value Error : line-height -1 negative values are not allowed
We could easily add the following:

if (parsed.value < 0) { throw error(`Value Error: line-height: ${parsed.value}; negative values are not allowed.`); }

jednano · 2018-08-14T23:13:34Z

src/helpers.ts

+	return false;
+}
+
+function isOnlyNumber(value: string): boolean {


parse-css-dimension might also be useful here.

I am not sure:
the "allow number only" parsing is not really provided by parse-css-dimension, or more precisely, it does not make sure that the input-string does not contain more than the number and/or the the unit

E.g. for "43asdfa7rem" it will return a valid result with number 43 and unit "rem"

Aha. Sounds like it needs a strict mode then. That can be arranged.

yes, then most of this PR should be removed (or moved to parse-css-dimension where appropriate) & replaced by using parse-css-dimension

Would you start on parse-css-dimension, e.g. how it should be invoked for "strict mode"?
e.g. maybe something like parseCssDimension('42dpi', 'strict') or maybe parseCssDimension('42dpi', {strict: true}), or ...?

I would then transfer/add parts from this PR for the strict-number testing as much as possible

I think we should go with an options hash of { strict: true }.

@russaa see jednano/parse-css-dimension#4. Also, I'm updating that project to TypeScript/AVA like this one. Feel free to submit a PR for jednano/parse-css-dimension#4.

jednano · 2018-08-14T23:14:34Z

src/index.test.ts

@@ -6,6 +6,57 @@ const fontWeightKeywords = require('css-font-weight-keywords');
 const fontStyleKeywords = require('css-font-style-keywords');
 const fontStretchKeywords = require('css-font-stretch-keywords');

+const numberValues = [


I think you'll find that many of these tests cases are covered here, so as not to reinvent the wheel.

jednano · 2019-01-28T03:51:46Z

Closing for now, due to inactivity.

jednano · 2019-01-29T05:52:00Z

@russaa I have a 🎁 for you: https://www.npmjs.com/package/parse-css-number

russaa · 2019-01-30T12:06:28Z

@jedmao very nice 😃 using JSON.parse 👍

added support for parsing arbitrary weight numbers and stretch percen…

267d5e7

…tages

jednano requested changes Aug 10, 2018

View reviewed changes

russaa added 5 commits August 13, 2018 21:16

refactor helper functions

91d049a

* extract regular expressions for reusing instances * split parse-as-number-only and parse-as-percent-number-only * decompose regular expression match

reformatted & removed unnecessary slice() operations

11ca8e8

refactored/reduced unnecessary else-clauses & removed unnecessary casts

cb1f195

require presence of at least one digit in numberPrefixPattern

39ba33b

BUGFIX use correct type RegExpExecArray instead of RegExpMatchArray

eaec645

jednano requested changes Aug 13, 2018

View reviewed changes

remove unnecessary else-statement & changed ts config to avoid unnece…

45a1db5

…ssary cast

jednano requested changes Aug 14, 2018

View reviewed changes

jednano mentioned this pull request Aug 16, 2018

Support { strict: true } option jednano/parse-css-dimension#4

Open

russaa mentioned this pull request Sep 5, 2018

strict mode for parsing numbers jednano/parse-css-dimension#6

Open

jednano closed this Jan 28, 2019

support for parsing arbitrary weight numbers and stretch percentages #11

support for parsing arbitrary weight numbers and stretch percentages #11

Uh oh!

Conversation

russaa commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jednano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jednano Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russaa commented Aug 13, 2018

Uh oh!

russaa commented Aug 13, 2018 • edited by jednano Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jednano left a comment

Choose a reason for hiding this comment

Uh oh!

jednano Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russaa commented Aug 14, 2018

Uh oh!

jednano left a comment

Choose a reason for hiding this comment

russaa commented Aug 10, 2018 •

edited

Loading

codecov bot commented Aug 10, 2018 •

edited

Loading

jednano Aug 13, 2018 •

edited

Loading

russaa commented Aug 13, 2018 •

edited by jednano

Loading

jednano Aug 13, 2018 •

edited

Loading