Closed kaushalmodi closed 4 years ago
So, I re-arranged things as discussed. The one change that bears repeating on top of the commit message and RELEASE_NOTES.md patches is that I renamed your clCfgToml
to cgCfgToml
('l'
-> 'g'
) which might be easy to miss since it's only a one-letter change but might break your build/testing.
It's probably about ready to release 0.9.46 which I would like to do soon to get any (probably negative) feedback before then releasing again as 1.0. But let's test it ourselves for the rest of the week before that 46 release.
Well, in light of the Nim CI being horribly broken and testing a mishmash of versions we should maybe release 0.9.46 sooner. But I'd like you to at least test the TOML stuff with my changes before I do that.
Yes, I am following that conversation. I can test in 40 minutes. Testing it yourself is easy too.. just nimble install parsetoml, place that example config.toml in your .config/cligen and compile with that define.
Before you release, why is the prefix "cl" used for the file clCfgInit.nim (I just copied that for TOML), but "cg" is used for everything else (vars, defines)? Should those "cl" prefixes in files be removed because they anyways live in "cligen" dir? If not, then have the same "cg" prefix as everything else?
cg for C)LI G)EN is used elsewhere. cligen/
is the package namespace and the variable name is clCfg
. So the files are <varName><operation>
. Arguably the global var should maybe have been called cgCfg
instead, but changing that now would break code and probably isn't worth that perturbation.
We could also change the convention for the cligen/*(Init|Toml)
to be just cligen/cfgInit.nim
and cligen/cfgToml.nim
. I don't really care that much one way or the other, but was just explaining my thinking.
It may inform the thought process here that clCfg
is just the global var for default. A CL author can have several such ClCfg
instances, init them however they want and pass them into the dispatch macros, etc. This is also a way for you to avoid repetitive passing in your dispatchMulti
to change usage templates/etc. although doing that now might block a CL user from modifying said templates to be colorized/whatever.
I have only used 1 instance, the default clCfg
. So does the clCfgInit
affects only that default object and not all? If so, I can understand the current names.
Final nitpick: change clCfgToml.nim
to clCfgInitToml.nim
?
Yeah. Just the default clCfg
.
If we did your final nitpick we'd want to rename the other one, too, to like clCfgInitParseCfg.nim
which is getting kind of ornate/wordy like the clCfgInitFromToml.nim
kind of school of identifiers. I don't think I'll ever be distributing a 3rd parser and the short name is fine. I mean, it's not like you do much with a config file besides init stuff. So, clCfgParseCfg.nim
is another idea (dropping the "Init"), but eh. That has its own little confusions. So, in short nothing really seems perfect and the current set up seems ok to me.
we'd want to rename the other one, too, to like clCfgInitParseCfg.nim
We are allowing that file to be overridden by the user in their projects. There, that file is not tied to use any parser. So clCfgInit.nim
makes sense there.
So, in short nothing really seems perfect and the current set up seems ok to me.
Sure :)
Another good point. One way to block CL uses modifying anything would be to create a zero-length cligen/clCfgInit.nim
. :-)
Someday, if the Nim stdlib inducts parsetoml
, I could be persuaded to maybe default to the TOML one. The end-CL user formats are so close that it probably wouldn't matter that much. But then we'd probably just delete (or replace by empty the clCfgTom.nim
and put the TOML code in clCfgInit.nim
and that name would still make sense.
Everything is working fine from HEAD. Just 1 pending renaming remaining in configs/README.md:
-d:clCfgToml
-> -d:cgCfgToml
Fixed.
Actually, if parsetoml
gets inducted into the Nim stdlib then we would be better off just having a search path that tried ".toml"
first, then without the ".toml"
and just dispatched to the appropriate parser. As long as any 3rd parties only replace via include cligen/clCfgInit.nim
then they'll be fine in that future day when we make the shipped-clCfgInit smart enough to parse either. Since the Init fork is the default that's almost surely what they'd pick, too.
Well, I just tested it with TOML mode, too and it worked fine. So, I think it's probably ready to release. Agree?
if parsetoml gets inducted into the Nim stdlib
I hope it does. But I doubt it because it kind of competes with the existing parsecfg stdlib.
we would be better off just having a search path that tried ".toml" first, then without the ".toml" and just dispatched to the appropriate parser.
Yes, I thought of that yesterday, but that would be elegant if that tailend code dealing with CLIGEN env var parsing. etc. is done DRY in a single wrapper file, and then calling one or the other apply
proc based on the found config. That certainly doesn't need to gate this release.
What bugs me about parsecfg
is the single-" inside a triple-""" does not get closed out. so you say foo = """.... "string with space"""
. I'm not sure if it's even possible to have >1 such string with spaces but none of my programs have so far needed >1 such string. I have comments about this in the example lc
and procs
configs.
The problem with that CLIGEN-driven dispatch now is that parsetoml
would still need to be installed just to compile anything. That's the only reason I don't "only use" that one. I feel kinda strongly that "just CLI parsing" is kind of "leaf package functionality" and should not imply even learning nimble. git clone cligen; add --path:wherever (on nim c cmd line or in a config) and you are good to go. We could organize it differently so that -d:cgCfgToml
just activated that CLIGEN-smarts, though.
What bugs me about parsecfg is the single-" inside a triple-""" does not get closed out. so you say foo = """.... "string with space"""
I don't understand the problem. Should that be a bug report for parsecfg?
The problem with that CLIGEN-driven dispatch now is that parsetoml would still need to be installed just to compile anything.
I didn't understand the "CLIGEN-driven dispatch" part? With the current code in master, cligen still compiles without parsetoml, right?
Probably should be a bug report for parsecfg
. Nim core has a lot of these DSLs that have, shall we say, fragile/incomplete parsers.
Sorry..a couple of "currently"s in play. The current code compiles fine. If we adapted the current CLIGEN code to dispatch based on run-time data like the filename found/$CLIGEN then it would need a when
guard. Then some error message and a qustion about if we even warn if config.toml
exists but the program was not compiled with -d:cgCfgToml
. Just various questions.
it would need a when guard. Then some error message and a qustion about if we even warn if config.toml exists but the program was not compiled with -d:cgCfgToml
I take care of that in my last PR :)
You seem to love strformat
and so you probably know about this recent fragile/incomplete parsing example, but just in case you didn't: https://forum.nim-lang.org/t/6250
You seem to love strformat
You got that right! :D A while back, I did some serious analysis of that module: https://scripter.co/notes/nim-fmt/
Yes, I saw that, but I haven't yet needed to have double quotes in my formatting 🤞
Yeah. For cligen
-proper configs it's unlikely. Colors are more useful. I only really do it for lc
/procs display
format strings.
Ref: https://github.com/c-blake/cligen/pull/140