Parse formatted number string to number #41
Conversation
1. Added new function joinParts to build number from parts 2. Cleaned up code in parseString as per feedback
value based on locale 1. Add new function parse to expose new feature 2. Add new function parseString in Parser.elm to implement parse 3. Add new function joinParts to join the number parts into float
…er into parse-string
|
@cuducos I have never used rebase on command line. I believe I have messed it up. Please squash the commits and merge if all changes are ok. |
| parse { base | thousandSeparator = ",", negativePrefix = "-" } "-100#000" | ||
| --> Just -100000 |
There was a problem hiding this comment.
Why the # in the -100#000 example? If # is not a thousand separators, this example seems wrong, right?
There was a problem hiding this comment.
Yes. It is wrong. But, this is the problem with current approach of cleaning integer part of non-digits. Thousand separator does not have any value in parsing.
There was a problem hiding this comment.
We can opt for a strategy to validate every every non-digit is one of the expected tokens (negative prefix or suffix, zero prefix or suffix, separators):
- Split the value by the regex
\d+ - Check if the resulting list contains only expected tokens
| {-| Given a `Locale` parses a `String` into a `Maybe Float` | ||
| -} |
There was a problem hiding this comment.
We need some examples here, both for documentation and for tests ; )
There was a problem hiding this comment.
Reason I did not add any tests here is because parse function in FormatNumber.elm is just calls this function and does nothing else. I thought it would be repetition.
There was a problem hiding this comment.
parse function in FormatNumber.elm is just calls this function
So, what about moving this function to FormatNumber.elm?
| onlyDigits = | ||
| Regex.replace notDigits (\_ -> "") | ||
|
|
||
| splitValue : String -> List String |
There was a problem hiding this comment.
I still think we should use String -> (String, String) here.
There was a problem hiding this comment.
If I change this, then I can move entire logic of joinParts into the parseString. I like the current structure as it cleanly segregates that out of parseString and allow me to test it separately.
Let me know if you still want it changed. I will do it.
There was a problem hiding this comment.
I think making impossible states impossible worth the job : )
|
@cuducos I believe there is a major flaw that I was not thinking about that is exposed by example: I have not built any code to validate the string against locale before parsing. I am working on a function to do the same. Will take little time but, I am hoping that would be a better approach. I will create a separate branch for the same so, we are able to evaluate that approach while building on the current code. |
This PR fixes #35. It is a draft PR to implement new feature to convert a formatted string to Maybe Float.
There is still some work in docs. Please review the approach and let me know your thoughts.