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
508 stars 24 forks source link

Clean up styleCheck warnings #112

Closed kaushalmodi closed 5 years ago

kaushalmodi commented 5 years ago

Hello,

When I compile any of my package using cligen with --styleCheck:hint, I get these warnings:

/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(565, 12) Hint: 'keyCount' is declared but not used [XDeclaredButNotUsed]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(462, 20) Hint: 'ForceParamDispatch' should be: 'forceParamDispatch' [Name]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(462, 20) Hint: 'ForceParamDefault' should be: 'forceParamDefault' [Name]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(462, 20) Hint: 'SubmittedChangelistsParamDispatch' should be: 'submittedChangelistsParamDispatch' [Name]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(462, 20) Hint: 'SubmittedChangelistsParamDefault' should be: 'submittedChangelistsParamDefault' [Name]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(565, 12) Hint: 'keyCount' is declared but not used [XDeclaredButNotUsed]
/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(565, 12) Hint: 'keyCount' is declared but not used [XDeclaredButNotUsed]

Can you please fix them? Once these are fixed, I plan to compile those packages with --styleCheck:error.

PS: Once you fix these, it might be helpful to put switch("styleCheck", "error") in a config.nims in the root of this repo.

c-blake commented 5 years ago

The [Name] warnings for *Param(Dispatch|Default) come from your parameter names (probably "Force" and "Submitted") not my code. If you want to be style safe then you probably want to change those to force and submitted and put in a short = {"force":'F', "submitted":'S'}.

The keyCount declared but not used is mine, but honestly I think it's probably bad form to strictly error out on declared but not used conditions. I can see them being unsightly/annoying, but they shouldn't be a show stopper. Or does --styleCheck:error fail on those, too?

It's often a PITA in code generators to never declare anything unless you'll really need it in later generated code. I've noticed that keyCount and looked into it at some point, but don't recall why it was hard to fix. We can keep this issue open if that really matters to you.

kaushalmodi commented 5 years ago

The [Name] warnings for *Param(Dispatch|Default) come from your parameter names

Oops! You are correct!

Or does --styleCheck:error fail on those, too?

I don't think --styleCheck:error will fail those; you can try it out.

kaushalmodi commented 5 years ago

We can keep this issue open if that really matters to you.

I am closing this.. If I still cannot compile my projects with --styleCheck:error due to an error in cligen, I'll reopen this. Next time, I'll make sure that it's not a dynamically generated var from my project :)

c-blake commented 5 years ago

No worries. I tried to pick my "suffixes" (e.g. ParamDispatch) for generated names to try to be recognizable. It worked! But maybe only for me as a reader. :-) Had you searched for "submitted" you would find the string nowhere in the cligen repo. That could also have been a tip off.

kaushalmodi commented 5 years ago

Had you searched for "submitted" you would find the string nowhere in the cligen repo.

Yes, I was lazy..

Even if I had just read (not even searched) "SubmittedChangelists", I would have known that it came from my perforce/p4 package.

c-blake commented 5 years ago

Wow. Opening an issue with copy-pasted errors to complain about said errors without actually even reading the names being complained about...That is lazy. It's ok, though, at least according to Larry Wall http://threevirtues.com/ :-)

kaushalmodi commented 5 years ago

Little bit credit for at least opening the issue? :P

I just copied all lines with:

/home/kmodi/.nimble/pkgs/cligen-0.9.36/cligen.nim(565, 12) Hint: ..

kaushalmodi commented 5 years ago

btw I confirm that now the "'keyCount' is declared but not used" are the only warnings emanating from cligen.

c-blake commented 5 years ago

Cool. If you want to take a look at silencing the keyCount thing, I'd happily accept a PR. I'm in the middle of something else at the moment.

kaushalmodi commented 5 years ago

I did a quick grep and found it amusing that you have that warning as part of your test suite :D

https://github.com/c-blake/cligen/blob/31fdd7040d8bc0d930b15d28acb0149646883489/test/ref#L673-L674

I'd happily accept a PR

Working on it ..

Update: OK, this wasn't as simple as I thought.. the keyCount identifier is probably created on the fly using macros.. so grep doesn't help there. I'll need to revisit this later.

c-blake commented 5 years ago

If all that really matters is silencing that pedantic warning (yeah..been there forever and I got sick of seeing it in the test diff), we could just always "use" the variable trivially as in keyCount.inc "Kaushal"; keyCount.dec "Kaushal" right after we declare it. That might slow down program startup by like, oh, 20-40 nanoseconds or some similar hard to measure amount and Nim is unlikely to ever get an analyzer smart enough to realize that such a use was a no-op.

c-blake commented 5 years ago

Maybe you can confirm all your own warnings go away? The one in the test suite did.

kaushalmodi commented 5 years ago

Oh, I was submitting the PR and then realized why it complained of merging conflict: https://github.com/kaushalmodi/cligen/commit/dabbe7bace487e51eb1b3661ee637fc2c6269208

Can you also fix the https://github.com/kaushalmodi/cligen/commit/dabbe7bace487e51eb1b3661ee637fc2c6269208#diff-5cbfc5f8e7ea060286d1849ee868c3deR871 part?

c-blake commented 5 years ago

Done.

kaushalmodi commented 5 years ago

Thanks! cligen is now all clear.. now to deal with the styleCheck warnings coming from critbits ...

c-blake commented 5 years ago

Yeah. I know about those, but that seemed more for style czars over at Nim stdlib. :-) I use that for quite a few things in both cligen and lc. So, hopefully you won't break anything. :-) I found some bug that wasn't caught by the unit tests probably caused by the '\0' no longer being visible from Nim '[]'. I just hope they don't change styleCheck rules very often...That would be a drag. Cheers.

kaushalmodi commented 5 years ago

Ref: https://github.com/nim-lang/Nim/pull/11752

OK enough style fixing for the day..

c-blake commented 5 years ago

:-)