Docs for most public functions for formatting DateTimes and Numbers #73
Conversation
src/Data/Formatter/DateTime.purs
Outdated
| format ∷ Formatter → DT.DateTime → String | ||
| format f d = foldMap (formatCommand d) f | ||
|
|
||
| -- | Format a DateTime according to the format defined in the given format string. | ||
| -- | If the format string is empty, will return a `Left` value. Note that any |
There was a problem hiding this comment.
I could use a spot check on that statement. From what I could tell, the only situation that would return a Left value was if the string is empty (from List.some on 164), but literally anything else would parse as a Formatter and just have Placeholder for any unrecognized portions of the input string.
There was a problem hiding this comment.
hmmm, I ran this:
quickCheck' 100_000 \str ->
(if str == "" then true else isRight $ parseFormatString str)
<?> "Test failed for input '" <> str <> "'"and got
Test failed for input 'ᆌH졶簮'
So any advice what this message should say?
…ct an inaccuracy in one of the doc comments
src/Data/Formatter/DateTime.purs
Outdated
| -- | Format a DateTime according to the format defined in the given format string. | ||
| -- | If the format string is empty, will return a `Left` value. Note that any | ||
| -- | unrecognized character is treated as a placeholder, so while "yyyy-MM-dd" might | ||
| -- | not produce the format you want (since "yyyy" and "dd" aren't recognized formats), |
There was a problem hiding this comment.
This statement would become inaccurate if #37 were implemented and merged.
There was a problem hiding this comment.
I'll go ahead and change that too while we're sanding off rough edges. The comment still holds true for yyyy though.
There was a problem hiding this comment.
I'll update this comment with the assumption that "d" will be an accepted format then.
Conflicts: src/Data/Formatter/DateTime.purs src/Data/Formatter/Number.purs
src/Data/Formatter/DateTime.purs
Outdated
| -- | Format a DateTime according to the format defined in the given format string. | ||
| -- | If the format string is empty, or contains astral plane characters (i.e., unicode | ||
| -- | code points that aren't representable in a single code unit), will return a `Left` value. |
There was a problem hiding this comment.
I could use a spot check on this statement. I originally thought that only empty strings could produce a Left value, but checked that with QuickCheck, and got "ᆌH졶簮" as another value of an input that produces Left. I'm pretty sure this is because those are astral plane characters that aren't representable via Char, which is used in placeholderContent up on 132. But honestly, I'm not the most familiar with unicode and code units vs. code points.
There was a problem hiding this comment.
I feel like I should fix the parser to support a better unicode range rather than having to do documentation gymnastics to excuse it 😄. Let's remove the statement, and I'll take a look at fixing it.
There was a problem hiding this comment.
Hah! It took a minute to figure it out, as I was going down the unicode track, but the reason that input fails is because of the H in there - it's a partial formatter command.
The parser is expecting HH, so throwing a failure when it fails to see another H after the first one.
There was a problem hiding this comment.
🤦♂️
ugh, that's embarrassing. Okay I'll update this.
src/Data/Formatter/DateTime.purs
Outdated
| -- | | ||
| -- | while `printFormatter (Hours24 : MinutesTwoDigits : Nil) = "HHmm"`. | ||
| -- | | ||
| -- | The interpretation of the format string is inspired by [momentjs](https://momentjs.com/docs/#/displaying/format/). |
There was a problem hiding this comment.
I'd suggest we just mention this somewhere once, rather than on every function that deals with a format string. Probably in the same place we explain the format!
We could perhaps the format info we have in the README just now in a module header comment, as that way it'll be contextually available in the pursuit docs too.
src/Data/Formatter/DateTime.purs
Outdated
| -- | Format a DateTime according to the format defined in the given format string. | ||
| -- | If the format string is empty, or contains astral plane characters (i.e., unicode | ||
| -- | code points that aren't representable in a single code unit), will return a `Left` value. |
There was a problem hiding this comment.
I feel like I should fix the parser to support a better unicode range rather than having to do documentation gymnastics to excuse it 😄. Let's remove the statement, and I'll take a look at fixing it.
src/Data/Formatter/DateTime.purs
Outdated
| -- | Format a DateTime according to the format defined in the given format string. | ||
| -- | If the format string is empty, will return a `Left` value. Note that any | ||
| -- | unrecognized character is treated as a placeholder, so while "yyyy-MM-dd" might | ||
| -- | not produce the format you want (since "yyyy" and "dd" aren't recognized formats), |
There was a problem hiding this comment.
I'll go ahead and change that too while we're sanding off rough edges. The comment still holds true for yyyy though.
src/Data/Formatter/Number.purs
Outdated
| @@ -63,6 +74,8 @@ instance showFormatter :: Show Formatter where | |||
|
|
|||
| derive instance eqFormatter :: Eq Formatter | |||
|
|
|||
| -- | The format string representation of a `Formatter`. | |||
| -- | The interpretation of the format string inspired by [numeral.js](http://numeraljs.com/#format) | |||
There was a problem hiding this comment.
Same thing applies here as to the comment about moment.js. Also this information is news to me, I didn't know where it came from 😄 (I didn't implement this module).
|
🤦♂️ I thought I'd already returned to this. Thanks very much for working on it an apologies about the extremely slow turnaround on my part! |
|
No worries! Glad to help out. |
Description of the change
This PR just adds documentation for most of the existing public functions in the Data.Formatter.DateTime and Data.Formatter.Number modules. Intended to help with #60, though I don't know if it's fair to say it fixes it. I aired on the verbose side, so let me know if I need to do some pruning.
Checklist: