purescript-contrib / purescript-formatters

Formatting and printing for numeric and date/time/interval values
Apache License 2.0
41 stars 30 forks source link

Fix math imports; drop Math #80

Closed JordanMartinez closed 2 years ago

JordanMartinez commented 2 years ago

Description of the change

Drops math; updates imports


Checklist:

thomashoneyman commented 2 years ago

Bit of an interesting CI error here.

JordanMartinez commented 2 years ago

Huh... Yeah it is. I'll look into it.

JordanMartinez commented 2 years ago

The failing test is caused by Nate's changes in purescript-parsing, specifically choice: https://github.com/purescript-contrib/purescript-parsing/commit/f50e8dd01d09d619a837e502b0c82d5d84a82844#diff-d472726f9da7dc968f3c6ef534ba99b06228789ab00a1f2dfe04eebfde42ffc6L445-R445

Changing this line (https://github.com/purescript-contrib/purescript-formatters/blob/main/src/Data/Formatter/Parser/Interval.purs#L32=) to the following makes the test pass again:

- parseInterval duration date = [ startEnd, durationEnd, startDuration, durationOnly ] <#> PC.try # 
+ parseInterval duration date = [ startEnd, durationEnd, startDuration, durationOnly ] <#> PC.try # foldl (<|>) empty

Conceptually, the parse is the same. The difference is what the final parser is and how that affects the error message:

-- choice == foldr (<|>) empty
a <|> b <|> c <|> empty

-- choice == foldl (<|>) empty
empty <|> a <|> b <|> c

@natefaubion I believe you modified choice because <|> hadn't yet been made right-associative. Now that it is, should choice be reverted back to foldl (<|>) empty?

natefaubion commented 2 years ago

I think choice should still be right associative. The fact that choice required you to always step through a useless alternative is an odd requirement.

JordanMartinez commented 2 years ago

K, I'll update the error message then.

JordanMartinez commented 2 years ago

Can I get an approval?

natefaubion commented 2 years ago

Sorry, I'm not saying that the current error behavior is OK, I'm just saying that I would like to preserve both. I'll need to think about it.