-
Notifications
You must be signed in to change notification settings - Fork 21
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 rhine-cassava #226
base: master
Are you sure you want to change the base?
Add rhine-cassava #226
Conversation
turion
commented
May 15, 2023
•
edited
Loading
edited
- Clean up git history
- Add a few more CSV tests
- Add a few file unit tests
- Use in a bigger example
- Add CSV writing tools
- Timestamping from CSVs, where one can maybe specify the timestamp column
Consider adding a |
4a70b70
to
1fec3b7
Compare
7a29d5c
to
3e714b1
Compare
-} | ||
newtype CsvClock = CsvClock (File String (Vector Record)) | ||
|
||
-- | Create a 'CsvClock' from a file path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document how the CSV file needs to be formatted, probably in the module doc
|
||
Additionally to an end-of-file error, it can also return a parse error as a 'String'. | ||
-} | ||
newtype CsvClock = CsvClock (File String (Vector Record)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming: Better call it Csv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit inconsistent in the code base. I think I've sometimes called something e.g. SelectClock
to avoid clashing with Select
(an obscure monad transformer). But in principle it would be nicer not to have Clock
in the name every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Csv
clashes prominently with a type of the same name from cassava
. I could call it CsvRow
or something like that. But CsvClock
is more memorable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CsvRecord
? And a corresponding better name for the parsing variant?
52a3f97
to
8af21fa
Compare
8af21fa
to
385798c
Compare
@@ -0,0 +1,45 @@ | |||
module FRP.Rhine.Clock.Csv ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a better hierarchy would be:
FRP.Rhine.Csv.Clock -- this module
FRP.Rhine.Csv -- Csv utils like CSV writer ClSF, and clock reexport
|
||
Additionally to an end-of-file error, it can also return a parse error as a 'String'. | ||
-} | ||
newtype CsvClock = CsvClock (File String (Vector Record)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the clock automatically did the parsing (and throws an error if it fails)
I've observed runtime errors with this approach in the wild. Possibly, the way I'm reading the file here isn't that good, and instead I should use the streaming interface https://hackage.haskell.org/package/cassava-0.5.3.0/docs/Data-Csv-Streaming.html#g:2 But this would mean not reusing the file clock. Which is fine. |
I definitely need hardened tests that would have detected these errors. |