Skip to content

Fix math imports; drop Math #80

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Mar 29, 2022
Merged

Fix math imports; drop Math #80

merged 4 commits into from
Mar 29, 2022

Conversation

JordanMartinez
Copy link
Contributor

Description of the change

Drops math; updates imports


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@thomashoneyman
Copy link
Contributor

Bit of an interesting CI error here.

@JordanMartinez
Copy link
Contributor Author

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

@JordanMartinez
Copy link
Contributor Author

The failing test is caused by Nate's changes in purescript-parsing, specifically choice: purescript-contrib/purescript-parsing@f50e8dd#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
Copy link
Collaborator

natefaubion commented Mar 28, 2022

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
Copy link
Contributor Author

K, I'll update the error message then.

@JordanMartinez
Copy link
Contributor Author

Can I get an approval?

@natefaubion
Copy link
Collaborator

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.

@JordanMartinez JordanMartinez merged commit 7e0251e into purescript-contrib:main Mar 29, 2022
@JordanMartinez JordanMartinez deleted the fix-math branch March 29, 2022 13:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants