-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trimming the prefix_by message while censoring warning #1194
Conversation
Should we give support for regex also? |
I'm not sure what this is addressing - do you have a concrete example? |
So I have this in my
and the warning message i want to handle is,
but the issue is somehow the warning message is padded with leading whitespace, so I had to change |
Ah that makes sense! Could you add a test for this? |
yes sure will add tests |
Added testcase |
test/Spago/Build.purs
Outdated
Spec.it "should censor warnings with given errorcode and prefix messsage" \{spago, fixture} -> do | ||
spago [ "init" ] >>= shouldBeSuccess | ||
let | ||
srcMain = Path.concat [ "src" , "Main.purs" ] | ||
spagoYaml = "spago.yaml" | ||
FSA.unlink srcMain | ||
FS.copyFile | ||
{ src : fixture "censor-warnings.purs" | ||
, dst : srcMain | ||
} | ||
FSA.unlink spagoYaml | ||
FS.copyFile | ||
{ src : fixture "censor-warnings.yaml" | ||
, dst : spagoYaml | ||
} | ||
spago [ "build" ] >>= shouldBeSuccess | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes:
- if you move the fixture files into a directory that is structures as a spago project, then you can use
FS.copyTree
to just copy the project in the test directory, so you don't have to callinit
and do this file juggling. See the fixtures for the monorepo tests for an example - the test is not checking that things are properly censored, just asserting that the build succeeds. We should check that we are not getting these warnings - see this test in the monorepo suite that is doing a similar thing:
spago/test/Spago/Build/Monorepo.purs
Lines 124 to 135 in fdaa7e6
Spec.it "build succeeds when 'strict: true' because warnings were censored" \{ spago, fixture } -> do | |
FS.copyTree { src: fixture "monorepo/strict-true-censored-warnings", dst: "." } | |
let | |
paths = | |
[ escapePathInErrMsg [ "package-a", "src", "Main.purs:6:13" ] | |
, escapePathInErrMsg [ "package-b", "src", "Main.purs:6:13" ] | |
, escapePathInErrMsg [ "package-c", "src", "Main.purs:6:13" ] | |
] | |
shouldNotHaveWarning stdErr = do | |
when (Array.any (\exp -> String.contains (Pattern exp) stdErr) paths) do | |
Assert.fail $ "STDERR contained one or more texts:\n" <> show paths <> "\n\nStderr was:\n" <> stdErr | |
spago [ "build" ] >>= check { stdout: mempty, stderr: shouldNotHaveWarning, result: isRight } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay this one looks neat, will do this👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes in the test cases, can you check once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
One of the ways to censor warnings is by using
by_prefix
. Some warning messages have additional leading whitespaces so the filtering is not happening correctly, so this PR fixes this by doing the filtering after trimming the text.TODO
Checklist:
README
P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊