[#56] Dump all the errors from different files #141

Sereja313 · 2022-09-13T16:56:26Z

Description

Problem: Currently, xrefcheck fails immediately after the first observed error because die is used right in markdownScanner What we want is dumping all the errors from different markdowns and then print them as a final xrefcheck's result together with the broken links. Also, despite the fact that in the makeError function we have 4 error messages, 2 of them are not reported, and the test case that should check this only checks that at least one of the four files throws an error.

xrefcheck/tests/Test/Xrefcheck/IgnoreAnnotationsSpec.hs

Lines 18 to 20 in 1b0b8d5

    
           it "Check if parsing incorrect markdowns produce exceptions" $ do 
        
             areIncorrect <- mapM (isIncorrectMD GitHub) failPaths 
        
             or areIncorrect `shouldBe` True

Solution: Make xrefcheck to report all errors. Add ScanError type and propagate errors to report all of them, rather than failing immediately after the first error is detected.

Related issue(s)

Fixes #56

✅ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

Tests
- If I added new functionality, I added tests covering it.
- If I fixed a bug, I added a regression test to prevent the bug from
  silently reappearing again.
Documentation
- I checked whether I should update the docs and did so if necessary:
  - README
  - Haddock
Public contracts
- Any modifications of public contracts comply with the Evolution
  of Public Contracts policy.
- I added an entry to the changelog if my changes are visible to the users
  and
- provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

My commits comply with the policy used in Serokell.
My code complies with the style guide.

tests/golden/check-scan-errors/expected.gold

src/Xrefcheck/Command.hs

src/Xrefcheck/Scanners/Markdown.hs

tests/golden/check-scan-errors/expected.gold

dcastro · 2022-09-17T11:33:28Z

Also, despite the fact that in the makeError function we have 4 error messages, 2 of them are not reported, and the test case that should check this only checks that at least one of the four files throws an error.

Great catch! 👏 Thank you for fixing this and adding these tests, it really helped uncover lots of issues with the existing code, well done!

src/Xrefcheck/Scanners/Markdown.hs

dcastro · 2022-09-22T13:39:27Z

tests/Test/Xrefcheck/IgnoreAnnotationsSpec.hs

+    it "Check if ignore link annotation produce error when placed at the end of file" do
+      let file = "tests/markdowns/with-annotations/no_link_eof.md"
+      getErrs file `shouldReturn`
+        makeError (Just $ PosInfo 9 1 9 31) file Link ""
+    it "Check if ignore paragraph annotation produce error when placed at the end of file" do
+      let file = "tests/markdowns/with-annotations/no_paragraph_eof.md"
+      getErrs file `shouldReturn`
+        makeError (Just $ PosInfo 9 1 9 36) file Paragraph "EOF"


I think it would be even better if these 2 new tests were written in bats instead, because then we can see whether the error message printed to stdout is sensible.

Tried this manually, it seems there's a double space in "but found^^EOF".

dcastro

Apart from that single comment above, LGTM, so I'm pre-approving 😄

Please merge after handling that comment, and I'll cut the 0.2.1 release 🎉

Sereja313 · 2022-09-22T21:21:12Z

@dcastro looks like readDirectoryWithL scans files in a different order on different machines(test in bf5501e works for me but doesn't work in CI and vice versa for 069434e).
Do I need to sort scan errors (or perhaps file paths in a directory tree before scanning) to make tests reproducible?

dcastro · 2022-09-23T06:14:43Z

@Sereja313 Sounds like a good idea!

sort scan errors (or perhaps file paths in a directory tree before scanning)

Whichever way you prefer

Problem: Currently, xrefcheck fails immediately after the first observed error because `die` is used right in `markdownScanner` What we want is dumping all the errors from different markdowns and then print them as a final xrefcheck's result together with the broken links. Also, despite the fact that in the `makeError` function we have 4 error messages, 2 of them are not reported, and the test case that should check this only checks that at least one of the four files throws an error. Solution: Make xrefcheck to report all errors. Add `ScanError` type and propagate errors to report all of them, rather than failing immediately after the first error is detected.

Sereja313 · 2022-09-23T07:18:19Z

Done 🎉

dcastro · 2022-09-23T11:08:42Z

Great work @Sereja313! 😄 🎉

Sereja313 requested a review from dcastro September 13, 2022 16:57

dcastro requested changes Sep 17, 2022

View reviewed changes

dcastro requested a review from Sorokin-Anton September 17, 2022 11:25

dcastro requested changes Sep 17, 2022

View reviewed changes

src/Xrefcheck/Scanners/Markdown.hs Outdated Show resolved Hide resolved

tests/golden/check-scan-errors/expected.gold Show resolved Hide resolved

dcastro reviewed Sep 17, 2022

View reviewed changes

src/Xrefcheck/Scanners/Markdown.hs Outdated Show resolved Hide resolved

src/Xrefcheck/Scanners/Markdown.hs Outdated Show resolved Hide resolved

Sereja313 requested a review from dcastro September 19, 2022 06:37

Sereja313 mentioned this pull request Sep 19, 2022

Refactor IgnoreMode #151

Closed

dcastro reviewed Sep 19, 2022

View reviewed changes

Sorokin-Anton approved these changes Sep 20, 2022

View reviewed changes

Sereja313 requested a review from dcastro September 20, 2022 22:10

dcastro reviewed Sep 22, 2022

View reviewed changes

dcastro approved these changes Sep 22, 2022

View reviewed changes

Sereja313 force-pushed the Sereja313/#56-dump-all-errors-from-files branch from 185a752 to bf5501e Compare September 22, 2022 20:01

Sereja313 force-pushed the Sereja313/#56-dump-all-errors-from-files branch from ea21a9e to c8d19a3 Compare September 23, 2022 07:15

Sereja313 merged commit d3b1cb5 into master Sep 23, 2022

Sereja313 deleted the Sereja313/#56-dump-all-errors-from-files branch September 23, 2022 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#56] Dump all the errors from different files #141

[#56] Dump all the errors from different files #141

Sereja313 commented Sep 13, 2022 •

edited

Loading

dcastro commented Sep 17, 2022

dcastro Sep 22, 2022

dcastro Sep 22, 2022

dcastro left a comment

Sereja313 commented Sep 22, 2022

dcastro commented Sep 23, 2022

Sereja313 commented Sep 23, 2022

dcastro commented Sep 23, 2022 •

edited

Loading

	it "Check if parsing incorrect markdowns produce exceptions" $ do
	areIncorrect <- mapM (isIncorrectMD GitHub) failPaths
	or areIncorrect `shouldBe` True

[#56] Dump all the errors from different files #141

[#56] Dump all the errors from different files #141

Conversation

Sereja313 commented Sep 13, 2022 • edited Loading

Description

Related issue(s)

✅ Checklist for your Pull Request

Related changes (conditional)

Stylistic guide (mandatory)

dcastro commented Sep 17, 2022

dcastro Sep 22, 2022

Choose a reason for hiding this comment

dcastro Sep 22, 2022

Choose a reason for hiding this comment

dcastro left a comment

Choose a reason for hiding this comment

Sereja313 commented Sep 22, 2022

dcastro commented Sep 23, 2022

Sereja313 commented Sep 23, 2022

dcastro commented Sep 23, 2022 • edited Loading

Sereja313 commented Sep 13, 2022 •

edited

Loading

dcastro commented Sep 23, 2022 •

edited

Loading