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

allow the directory that coverage reports are outputted to to be configured #145

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Jan 19, 2016

fixes #63

reviewers: @jbcpollak, @mateatslc, @jamestalmage

@jamestalmage
Copy link
Member

The code looks fine. I'm still not sold as to why it's necessary. Do people have real reasons they need to rename the directory? I haven't heard anybody say "Without this option it's impossible/difficult to use NYC, because...".

This sort of option isn't bad in itself, but allowing them to accumulate unchecked just bloats the codebase, creates more potential failure points, and just slows development of meaningful features. If there is legitimate need, then by all means 👍.

@jbcpollak
Copy link

The reason I needed to relocate the output was to conform to the
requirements of our CI service (circleci) which needs reports in a specific
directory.

On Mon, Jan 18, 2016, 11:34 PM James Talmage [email protected]
wrote:

The code looks fine. I'm still not sold as to why it's necessary. Do
people have real reasons they need to rename the directory? I haven't heard
anybody say "Without this option it's impossible/difficult to use NYC,
because...".

This sort of option isn't bad in itself, but allowing them to accumulate
unchecked just bloats the codebase, creates more potential failure points,
and just slows development of meaningful features. If there is legitimate
need, then by all means [image: 👍].


Reply to this email directly or view it on GitHub
#145 (comment).

@jamestalmage
Copy link
Member

Sold. :shipit:

@mateatslc
Copy link

My reason for this is similar to @jbcpollak's, help CI process find the report. This does exactly what I needed, thanks!

bcoe added a commit that referenced this pull request Jan 21, 2016
allow the directory that coverage reports are outputted to to be configured
@bcoe bcoe merged commit b2cc94f into master Jan 21, 2016
@jamestalmage jamestalmage deleted the fix-63 branch January 21, 2016 04:14
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.

Specify output folder
4 participants