fixes #20149; fixes #16762; hintAsError and warningAsError now ignore foreign packages #20151
Conversation
|
note to future lookup: pre-existing but hintAsError/warningAsError seems not to give a correct exit code which makes testament stop to work. |
ringabout
changed the title
|
In terms of feedback: When on the branch and compiling with It no longer throws a compiler error in the system.nim or fatal.nim modules, which it previously did! |
|
Unrelated CI failure, ggplot breaks for a short while. |
|
Given that it's running through and looking good, is this PR just waiting for somebody else to look over it? |
|
Yeah, it is ready for review. Feel free to review. Remember to approve my PR if you think it is fine.
|
| @@ -531,8 +531,7 @@ proc liMessage*(conf: ConfigRef; info: TLineInfo, msg: TMsgKind, arg: string, | |||
| of warnMin..warnMax: | |||
| sev = Severity.Warning | |||
| ignoreMsg = not conf.hasWarn(msg) | |||
| if msg in conf.warningAsErrors: | |||
| ignoreMsg = false | |||
| if not ignoreMsg and msg in conf.warningAsErrors: | |||
There was a problem hiding this comment.
What meaning does it have for msg in conf.warningAsErrors to be true? What would a boolean that contained the result of that statement be accurately be named like?
There was a problem hiding this comment.
Maybe you could expand this test with an unused import to test that in a single go? Would only require e.g. import std/options in the file and --warningAsError:UnusedImport in trunner.nim
Sounds good to me. Feel free to add a test case in the following up prs.
Is there a way to test that the flag correctly prohibits compilation? Or would that be out of scope?
Yeah, there are tests for that. Using compiles should work for simple cases. If you can also go through the trouble of using execCmdEx and compare the outputs. Following-up prs are welcome!
There was a problem hiding this comment.
What meaning does it have for msg in conf.warningAsErrors to be true?
conf.warningAsErrors is a set of TMsgKind enum type. If msg in the conf.warningAsErrors, it means this hints/warnings are enabled for the file and you have already specified that hints/warnings should be errors.
For instance, for a module within the current package, XDeclaredButNotUsed is enabled. If you also specify --hintAsError:XDeclaredButNotUsed is true, in this case XDeclaredButNotUsed is the msg variable, --hintAsError:XDeclaredButNotUsed means XDeclaredButNotUsed is added to conf.warningAsErrors. So the statement "msg in conf.warningAsErrors" is true.
For a foreign package, XDeclaredButNotUsed is not enabled by default. Even if --hintAsError:XDeclaredButNotUsed is specified, the statement "msg in conf.warningAsErrors" is false.
| @@ -249,12 +249,18 @@ sub/mmain.idx""", context | |||
| let cmd = fmt"{nim} r -b:cpp --hints:off --nimcache:{nimcache} --warningAsError:ProveInit {file}" | |||
| check execCmdEx(cmd) == ("witness\n", 0) | |||
|
|
|||
| block: # bug #20149 | |||
| let file = testsDir / "misc/m20149.nim" | |||
| let cmd = fmt"{nim} r --hints:off --nimcache:{nimcache} --hintAsError:XDeclaredButNotUsed {file}" | |||
There was a problem hiding this comment.
I'll still approve this, but would like to note that this will break if nim or nimcache are not properly escaped.
|
Thanks for your hard work on this PR! Hint: mm: orc; threads: on; opt: speed; options: -d:release |
…Error now ignore foreign packages (nim-lang#20151) * fixes nim-lang#20149; hintAsError/warningAsError ignores foreign packages * add changelog * fixes the test * remove * fixes tests again * fix * I'm careless Co-authored-by: xflywind <[email protected]>
… foreign packages (#20151) * fixes #20149; hintAsError/warningAsError ignores foreign packages * add changelog * fixes the test * remove * fixes tests again * fix * I'm careless Co-authored-by: xflywind <[email protected]> (cherry picked from commit 641381e)
fixes #20149
fixes #16762