search

c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
509 stars 24 forks source link

cligen fails with devel #91

Closed genotrance closed 5 years ago

genotrance commented 5 years ago 
        ... /tmp/nimble_27847/githubcom_cblakecligengit_0.9.17/cligen.nim(673, 14) template/generic instantiation of `dispatchGen` from here
        ... /tmp/nimble_27847/githubcom_cblakecligengit_0.9.17/cligen/argcvt.nim(8, 24) Error: undeclared identifier: '$'

Despite being in Nim tests, this failure isn't caught.

genotrance commented 5 years ago

Per @Araq, $ is now in system/dollar.nim.

https://github.com/c-blake/cligen/blob/master/cligen/argcvt.nim#L8

narimiran commented 5 years ago

It is an easy fix:

from typetraits import `$` # needed for $T

should be removed, as this $ is now available by default, no need to import typetraits.

But we'll add it back to remain backward-compatible.

narimiran commented 5 years ago

But we'll add it back to remain backward-compatible.

Done. Should be fine now.

c-blake commented 5 years ago

This came up once before in December just prior to Nim 0.19.2 when @timotheecour had that $ moved to system. So, best to guard it here so that the core Nim crew can delete it someday.

c-blake commented 5 years ago

(That other time was this issue: https://github.com/c-blake/cligen/issues/84 ). Anyway, cligen should now work fine if you delete that export, but I don't know how many other packages might fail.

c-blake commented 5 years ago

I can say that searching through like 150 git clones of nim packages that I don't see a single other instance of the from.*typetraits.*import.*\$ pattern. I expect almost everyone just import typetraits in which case at worst it's an unneeded import. (Personally, I use that from..import form because I dislike unneeded imports and like to reinforce what modules various things live in. I even put // side-comments in C code for each #include saying which decls require the header, but that practice seems very rare.)