status-im / nim-confutils

Simplified handling of command line options and config files
Apache License 2.0
63 stars 16 forks source link

handle `terminalWidth` exception #80

Closed etan-status closed 1 year ago

etan-status commented 1 year ago

terminalWidth() may fail with a ValueError on out of range widths from environment variables. Provide a suitable fallback.

tersec commented 1 year ago

Both Nim 1.6 and Nim 2.0 appear to have this issue, because https://github.com/nim-lang/Nim/pull/21968 hadn't yet switched to parseSaturatedNatural.

But once that happens, https://github.com/nim-lang/Nim/blob/eaf89777236d939e5c21c5daee9cf96423dc11bc/lib/pure/parseutils.nim#L495-L517 shows that

proc parseSaturatedNatural*(s: string, b: var int, start = 0): int {.
    raises: [].} =

i.e. no such exceptions can be thrown, and this becomes dead code.

Is there a way to indicate removal of such code here when appropriate, i.e. the environment variable parsing uses parseSaturatedNatural and not parseInt?

etan-status commented 1 year ago

Not sure if possible to check how that function that we call is implemented, as this is not Nim version specific but within a NIm version.

Added the github link so that at least we see when it gets merged.

tersec commented 1 year ago

Reasonable enough, yeah.