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 fmtFloat to stdlib #766

Open
timotheecour opened this issue Jun 28, 2021 · 6 comments
Open

add fmtFloat to stdlib #766

timotheecour opened this issue Jun 28, 2021 · 6 comments

Comments

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Jun 28, 2021

That was my idea on the original PR too, but got chopped off as usual. 🤷

@timotheecour
Copy link
Owner Author

timotheecour commented Jun 28, 2021

the fact that there was so many review comments and commits in the PR (because of failing edge cases) played a role IMO in the closing of that PR.

But regardless, the feature is useful and (as you discovered yourself) is tricky to implement correctly, which makes it a great candidate for stdlib; we should address the concerns raised in the closing comments regarding API though.

@juancarlospaco
Copy link
Collaborator

My first commit was fast and easy to understand,
...then people requested to support whatever crossed their mind.

The problem is that adds more and more defensive programming,
maybe there needs to be 2 modes, via another proc or overload or a static argument on 1 proc,
a "strict" one thats efficient and tiny,
and another that parses/formats any random potato into a float.

@timotheecour
Copy link
Owner Author

timotheecour commented Jun 29, 2021

I'm not saying it's an easy problem; the API needs to satisfy both:

  • performance for the common case (eg english 12,345.78)
  • customizability to support alternate formats (eg german 12.345,78)
  • whether we should also support yet-weirder formats like indian format (12,34,56,789.0) is TBD
    (refs https://forum.nim-lang.org/t/8162#52487)
  • no-alloc (ie reuse a buffer)

decisions to make

  • which module? strutils already has formatEng, so it could make sense there
  • strformat shouldn't be bothered with custom float formats IMO
  • my preference would be a new dedicated module which could handle custom formatting and parsing of floats and integers => name need to be bikeshedded
  • multiple optional params vs single (optional) object for params; I find single object for params easier to deal with (eg for forwarding to other procs), and packed object (packed object / bitfields #741) would give highest performance anyways

API

it should be additive (analog to addFloat), which is the most composable/efficient, can be used in conjunction with sugar.dup

type OptFloatFmt = object
  sep: char ...
type OptFloatParse = object
  sep: char ...
proc addFloatCustom*(result: var string, a: SomeFloat, opt = initOptFloatFmt())
proc parseCustom*(ret: var SomeFloat, s: openArray[char], opt = initOptFloatParse()): int
  # returns nb of chars parsed in `s`

note

#741 could be used to define packed object (more efficient + syntax sugar) instead of OptFloatFmt, OptFloatParse

@juancarlospaco
Copy link
Collaborator

  • which module? strutils already has formatEng, so it could make sense there

strmisc says This module contains various string utility routines that are uncommonly used in comparison to the ones in strutils.

I should live there IMHO.

@timotheecour
Copy link
Owner Author

possibly

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

No branches or pull requests

2 participants