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

Benp/jshint violations #15

Merged

Conversation

benpatterson
Copy link

@Bachmann1234 mind taking a look?

@Bachmann1234
Copy link
Owner

Ill take a look today or tomorrow!

@benpatterson
Copy link
Author

yo @Bachmann1234 looks like I have broken tests :( I'll address those and let you know when it's GTG for you. Sorry about that...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) to 99.01% when pulling faa0820 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) to 99.01% when pulling faa0820 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) to 99.01% when pulling faa0820 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) to 99.01% when pulling faa0820 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.17%) to 99.06% when pulling ec1dfb8 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.17%) to 99.06% when pulling ec1dfb8 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

class JsHintQualityReporterTest(unittest.TestCase):

def setUp(self):

Copy link
Owner

Choose a reason for hiding this comment

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

delete this blank line. Sorry to flake8 you. Don't care about the long lines though (im looking at the diff-quality report :-P)

@Bachmann1234
Copy link
Owner

A couple trivial comments. Looks pretty good to me. Can you squash this down?

@benpatterson benpatterson force-pushed the benp/jshint-violations branch 2 times, most recently from 4eb748f to 68dc4a4 Compare June 2, 2015 12:15
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 99.26% when pulling 4eb748f on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 99.26% when pulling 4eb748f on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.31%) to 98.92% when pulling 68dc4a4 on benpatterson:benp/jshint-violations into 8ae1814 on Bachmann1234:master.

Because jshint is (at the moment) not available through a python
library/port, confirming that it is available at runtime needs to
happen through a subprocess call, rather than a python import. The base class
for the violations reporter now has a fallback option of checking for a given
reporter through the CLI; however, the implementing class must provide
the command that verifies it's installed. That command must exit 0 if the
tool is successfully installed. (For jshint, it is `jshint --version`).
@benpatterson
Copy link
Author

@Bachmann1234 Added tests, fixed up flake8, and squashed. Happy to iterate again if you've got additional comments.

@Bachmann1234
Copy link
Owner

Cool ill give this a last once over soon!

@Bachmann1234
Copy link
Owner

Lets do this! Thanks for the PR

Bachmann1234 added a commit that referenced this pull request Jun 3, 2015
@Bachmann1234 Bachmann1234 merged commit 220afe4 into Bachmann1234:master Jun 3, 2015
@Bachmann1234
Copy link
Owner

https://pypi.python.org/pypi/diff_cover ITS OUT! :-) Thanks again

@benpatterson
Copy link
Author

Awesome!!!

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

3 participants