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

Mechanism for excluding files and lines. #55

Closed
wants to merge 1 commit into from

Conversation

dcjones
Copy link

@dcjones dcjones commented Apr 21, 2015

This addresses #44 and #53. It allows a per-directory file called '.coverage.yml' that looks like

# exclude files in the directory with these names
exclude_files:
  - foo.jl
  - bar.jl

# exclude any lines that match any of the following regexes
exclude_patterns:
  - "\[NO COVERAGE\]"

@IainNZ
Copy link
Contributor

IainNZ commented Apr 21, 2015

Neat! What do you think @timholy ?

@timholy
Copy link
Member

timholy commented Apr 21, 2015

Seems worth having, thanks @dcjones. I'll be greedy, though, and suggest tests and documentation.

@IainNZ
Copy link
Contributor

IainNZ commented Apr 21, 2015

I'd also like that, but I'll also settle for just merging if thats too much bandwidth right now. Something in the README would be good enough.

I sense this is the start of something bigger though - a .package.yml type thing that can be used for not just Coverage.jl, but also PackageEvaluator and any other thing that comes up (Lint.jl? @tonyhffong)

@dcjones
Copy link
Author

dcjones commented Apr 21, 2015

I'll add some tests and documentation later. This isn't urgent for me, I just have some generated code that I feel is going to unfairly penalize my coverage.

I sense this is the start of something bigger though - a .package.yml type thing that can be used for not just Coverage.jl, but also PackageEvaluator and any other thing that comes up (Lint.jl? @tonyhffong)

I would like that, and going even further I'd like it if REQUIRE and tests/REQUIRE were in that file as well.

@sbromberger
Copy link
Contributor

+1 to a merge (ref: https://groups.google.com/d/msg/julia-users/OLEbyA_-43c/P3EzEU5AJQAJ). Thank you.

@IainNZ
Copy link
Contributor

IainNZ commented Aug 4, 2015

@sbromberger well you/me/@dcjones would need to rebase. If you are using Codecov, you can manually exclude files on the website.

@sbromberger
Copy link
Contributor

I'm using Coveralls. I guess I could try to rebase this.

@IainNZ
Copy link
Contributor

IainNZ commented Aug 4, 2015

I'm less keen on merging this than I was previously, as I think it makes the package substantially more complex than I think anyone is willing to maintain right now. Or if it is added, it is not documented and can be broken at any time, which the goal of getting a more standardized thing in Base.

@sbromberger
Copy link
Contributor

I'll not spend any more time on it then :) I think it's important to offer a way of telling Coveralls that certain blocks of code do not need to be tested, though.

As an aside, is it really of benefit to add CI into Base? It seems to me that this goes against the philosophy of "keep Base simple" - while CI is great for development, once you're in production you don't need it at all.

(Or did I misunderstand regarding "a more standardized thing in Base"?)

@IainNZ
Copy link
Contributor

IainNZ commented Aug 5, 2015

I mean a more standardized package metadata format in Base, that can have lots of info, not just this.

@ebalaban
Copy link

Any plans to incorporate these features? They would be quite useful.

@Evizero
Copy link

Evizero commented Aug 9, 2016

bump ! would love this

@IainNZ
Copy link
Contributor

IainNZ commented Aug 9, 2016

This package isn't really being actively maintained apart from bug requests, but I'd suggest that if you want it then you should implement it. I still agree with past-me that it should be wrapped into a wider package info file, but maybe I'm wrong about that.

@timholy
Copy link
Member

timholy commented Aug 9, 2016

It doesn't need fresh implementation; this is a PR, and it just needs rebasing (and likely, fixing up). However, up above it was stated that it wouldn't be merged, so that's what needs to be settled first.

Personally, I think this is useful functionality. I also confess that I'm struggling to imagine what a "wider package info file" would do/look like. If there aren't concrete suggestions, then perhaps one strategy would be to start the process here and then move out into a separate package once there's a concrete motive for generalization?

@IainNZ
Copy link
Contributor

IainNZ commented Aug 9, 2016

I certainly wouldn't stand in the way of a merge, to be clear, 2016-@IainNZ isn't maintaining this anymore, and 2015-@IainNZ had loftier goals apparently.

@IainNZ
Copy link
Contributor

IainNZ commented Jun 20, 2018

It is unmergeable in its current state.
Maybe you could take the code and update it in a new PR @DilumAluthge ?

@zundertj zundertj mentioned this pull request Mar 10, 2019
3 tasks
@DilumAluthge
Copy link
Member

I tried to update this PR, but I couldn’t get it working.

Should we close this PR in favor of #218?

@vtjnash vtjnash closed this Oct 15, 2019
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.

8 participants