Skip to content
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

add sec/min/hrs/hr/days/wk/wks/weeks to time string parsing #195

Merged
merged 4 commits into from
May 1, 2021

Conversation

ketzacoatl
Copy link
Contributor

it's a first pass, feedback/guidance on the details are welcome..

This aims to address #193.

@ketzacoatl
Copy link
Contributor Author

Need to address the charLiteral bit..

@ketzacoatl
Copy link
Contributor Author

@NorfairKing I'm not quite sure about this.. where is charLiteral coming from / what would you like to replace it with if switching to a string? I went to find my own replacement, but could not figure out which library you were getting charLiteral from.

@ketzacoatl
Copy link
Contributor Author

@NorfairKing, I looked a little closer, and I believe smos-report is using charLiteral from megaparsec? https://www.stackage.org/haddock/lts-17.7/megaparsec-9.0.1/Text-Megaparsec-Char-Lexer.html#g:3

That doc section notes:

Note that you can use this parser as a building block to parse various string literals

And also:

Performance note: the parser is not particularly efficient at the moment.

So I wonder which function you would recommend smos-report use here in place of charLiteral. Should we use parsec's stringLiteral? https://www.stackage.org/haddock/lts-17.7/parsec-3.1.14.0/Text-Parsec-Token.html#v:stringLiteral

@NorfairKing
Copy link
Owner

NorfairKing commented Mar 22, 2021

@ketzacoatl charLiteral comes from megaparsec:

- megaparsec
: http://hackage.haskell.org/package/megaparsec-9.0.1/docs/Text-Megaparsec-Char-Lexer.html#v:charLiteral
I think we can use string instead: http://hackage.haskell.org/package/megaparsec-9.0.1/docs/Text-Megaparsec-Char.html#v:string

However, more importantly: This is a change in a parser. Parsers are notoriously hard to get right, so we'll want to start by writing tests. You can add them here:

@ketzacoatl
Copy link
Contributor Author

Parsers are notoriously hard to get right, so we'll want to start by writing tests. You can add them here

I looked that over a few times, but am having a difficult time seeing it clearly, so I'm not even sure I can explain how that existing spec is testing timeP. Maybe we could meet up and review this together.

@NorfairKing
Copy link
Owner

Sure we can discuss it together!

@ketzacoatl ketzacoatl changed the base branch from master to development April 30, 2021 20:10
@ketzacoatl
Copy link
Contributor Author

@NorfairKing This is now ready for your review ;)

@NorfairKing
Copy link
Owner

LGTM

@NorfairKing NorfairKing merged commit 58de3cb into NorfairKing:development May 1, 2021
@ketzacoatl ketzacoatl deleted the patch-1 branch May 1, 2021 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants