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

Add Coverage reporting to coveralls.io #75

Merged
merged 8 commits into from
Aug 3, 2017
Merged

Add Coverage reporting to coveralls.io #75

merged 8 commits into from
Aug 3, 2017

Conversation

cg505
Copy link
Contributor

@cg505 cg505 commented Aug 3, 2017

https://coveralls.io/github/kotct/dot

Also add badges for travis and coveralls to the readme.

This is a part of #53.

@cg505 cg505 requested review from rye and samontea August 3, 2017 00:33
@rye rye mentioned this pull request Aug 3, 2017
4 tasks
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c1b13d6 on coverage-reporting into ** on master**.

2 similar comments
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c1b13d6 on coverage-reporting into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c1b13d6 on coverage-reporting into ** on master**.

@cg505
Copy link
Contributor Author

cg505 commented Aug 3, 2017

what

@rye rye added this to the Version 0.2.0 milestone Aug 3, 2017
@cg505
Copy link
Contributor Author

cg505 commented Aug 3, 2017

@rye @samontea actually ready for review, sorry for the confusion

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f18f775 on coverage-reporting into ** on master**.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 3, 2017

Coverage Status

Changes Unknown when pulling f18f775 on coverage-reporting into ** on master**.

README.md Outdated
@@ -1,5 +1,7 @@
# kotct/dot

[![Build Status](https://travis-ci.org/kotct/dot.svg?branch=master)](https://travis-ci.org/kotct/dot) [![Coverage Status](https://coveralls.io/repos/github/kotct/dot/badge.svg)](https://coveralls.io/github/kotct/dot)
Copy link
Member

Choose a reason for hiding this comment

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

Can these go on the same line as the title?

.travis.yml Outdated
- emacs --script .emacs.d/test/lisp/run-tests.el
- emacs --script ~/.emacs.d/init.el
Copy link
Member

Choose a reason for hiding this comment

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

Suggest changing both to ~/.

- emacs --script .emacs.d/test/lisp/run-tests.el
- emacs --script .emacs.d/init.el
Copy link
Member

Choose a reason for hiding this comment

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

Use ~/ for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a discussion about this already, let me find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh, my apologies, I thought there was a ~/ at the start of this path and I was very confused.

;; override a local function that fixes this
(advice-add
#'undercover--wildcards-to-files :override
(lambda (wildcards)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't wildcards unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm overriding a function that takes an argument so I need to allow an argument to be passed.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, does the argument need to be the same though? (Like, can you do (lambda (_) on this line instead? Or (_wildcards)?) I haven't tested this but we might get lexical binding errors, see EL manual § 11.9.4 "Using Lexical Binding":

(To silence byte-compiler warnings about unused variables, just use a variable name that start with an underscore. The byte-compiler interprets this as an indication that this is a variable known not to be used.)

If it's not used, changing it to underscore would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use lexical binding in our code, so there's no way for emacs to know if a variable is unused. That said, it probably would be good style to put the underscore anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Well, style was what I was more concerned about anyways.

@@ -4,4 +4,7 @@
(package-initialize)
(package-refresh-contents)

;; add undercover for coverage reporting
(setf kotct/dependency-list (cons 'undercover kotct/dependency-list))
Copy link
Member

Choose a reason for hiding this comment

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

Erm. Any particular reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So undercover is installed, but only on Travis (via install script instead of on init).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see.

@cg505 cg505 requested a review from rye August 3, 2017 01:29
@cg505 cg505 merged commit 0875d0b into master Aug 3, 2017
@cg505 cg505 deleted the coverage-reporting branch August 3, 2017 01:32
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.

3 participants