search

projectfluent / fluent

Fluent — planning, spec and documentation
https://projectfluent.org
Apache License 2.0
1.42k stars 45 forks source link

Add more cases to validation. #286

Closed Pike closed 5 years ago

Pike commented 5 years ago

This is adding more options to more combinators. There's one thing I didn't find out how to validate, and that repeat1 should cover 1 and 2, and I don't think that's possible to validate. I tried tracing the production calls in a different patch, but that didn't return anything useful as it doesn't count if the 2-match ends up in a parser error somewhere else in the message.

@SirNickolas, can you give this a look? Also, Zibi?

SirNickolas commented 5 years ago

Looks fine to me, thanks.

There's one thing I didn't find out how to validate, and that repeat1 should cover 1 and 2, and I don't think that's possible to validate.

repeat1 → repeatExactly1, where repeatExactly1 = parser => parser.map(Array.of)? This will give failures if a case with more than one element is not tested. Or have I misunderstood something?

One more thing that is not completely related to this PR. I suppose, in select_expressions.ftl, names for tests invalid-selector-select-expression and invalid-selector-nested-expression are mixed up.

Pike commented 5 years ago

Looks fine to me, thanks.

There's one thing I didn't find out how to validate, and that repeat1 should cover 1 and 2, and I don't think that's possible to validate.

repeat1 → repeatExactly1, where repeatExactly1 = parser => parser.map(Array.of)? This will give failures if a case with more than one element is not tested. Or have I misunderstood something?

I wish I could. I tried, and just have to admit that s.tas is the only one that actually understands the combinator code to the extent that one could write new ones.

One more thing that is not completely related to this PR. I suppose, in select_expressions.ftl, names for tests invalid-selector-select-expression and invalid-selector-nested-expression are mixed up.

Right, fixing that up here, too.