Skip to content
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

Avoid failing on source-file parsing errors #42

Closed
wants to merge 2 commits into from
Closed

Conversation

timholy
Copy link
Member

@timholy timholy commented Nov 23, 2020

Fixes https://github.com/JuliaCI/Coverage.jl/issues/299. I'm not certain this is desirable, but...

Fixes #299. I'm not certain this is desirable, but...
src/CoverageTools.jl Outdated Show resolved Hide resolved
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
@DilumAluthge
Copy link
Member

I like the idea of printing a more informative message, explaining that there was a parsing error, and printing the name of the source file that had the parsing error.

I'm not sure I like the idea of completely suppressing the error. What if, after printing the informative message, you rethrow the error?

@timholy
Copy link
Member Author

timholy commented Nov 23, 2020

I'd wondered the same thing. I'd be happy to change it if that's your preference.

@DilumAluthge
Copy link
Member

Hmmm. Let's see what Jameson and Max think.

@DilumAluthge
Copy link
Member

Out of curiosity, is there a way for us to figure out what line the parsing error happened on? That would make it even easier for users to debug.

@coveralls
Copy link

coveralls commented Nov 23, 2020

Pull Request Test Coverage Report for Build 78

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 97.268%

Totals Coverage Status
Change from base Build 75: 0.02%
Covered Lines: 178
Relevant Lines: 183

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 77

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 97.268%

Totals Coverage Status
Change from base Build 75: 0.02%
Covered Lines: 178
Relevant Lines: 183

💛 - Coveralls

amend_coverage_from_src!(fc)
catch err
@error "coverage could not incorporate source file $filename, it may have a parsing error" exception=(err,catch_backtrace())
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
end
rethrow()
end

@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #42 (829a4fa) into master (47f880e) will decrease coverage by 0.41%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   96.41%   96.00%   -0.42%     
==========================================
  Files           4        4              
  Lines         223      225       +2     
==========================================
+ Hits          215      216       +1     
- Misses          8        9       +1     
Impacted Files Coverage Δ
src/CoverageTools.jl 97.45% <66.66%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47f880e...829a4fa. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Nov 23, 2020

Out of curiosity, is there a way for us to figure out what line the parsing error happened on? That would make it even easier for users to debug.

#43

@fingolfin
Copy link
Member

Both this and #43 would improve things, although PR #43 indeed seems more useful.

@DilumAluthge
Copy link
Member

I prefer #43

@fingolfin fingolfin closed this in #43 Dec 2, 2020
@DilumAluthge DilumAluthge deleted the teh/softerr branch June 8, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants