Skip to content

Conversation

@victor-torres
Copy link
Contributor

you can test with the command py.test tests/client/test_utils.py

I'm also adding requirements-base.txt to the requirements-test.txt file as we need some libs installed from this file to perform some tests

This fixes issue #103

you can test with the command 'py.test tests/client/test_utils.py'

I'm also adding requirements-base.txt to the requirements-test.txt file as we need some libs installed from this file to perform some tests
@@ -1,3 +1,5 @@
-r requirements-base.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the general idea here is:

  • when you need to simply install a package, you install everything from requirements.txt
  • when you want to install test-related packages, you install it additionally, with separate pip install -r cmd

So for me it's a matter of taste and I don't see any strong reason to change it.
Do you agree or I'm missing anything else here?

Copy link
Member

Choose a reason for hiding this comment

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

I also think this line is not needed. Also, it is a different discussion/feature than the one intended for this PR. I would remove it.

Copy link
Contributor

@vshlapakov vshlapakov left a comment

Choose a reason for hiding this comment

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

I'm not convinced about the change in requirements-test.txt and would prefer to drop it.
Apart from that, LGTM, very neat changes 👍

@codecov
Copy link

codecov bot commented Oct 26, 2018

Codecov Report

Merging #104 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   93.39%   93.41%   +0.01%     
==========================================
  Files          28       28              
  Lines        1878     1883       +5     
==========================================
+ Hits         1754     1759       +5     
  Misses        124      124
Impacted Files Coverage Δ
scrapinghub/client/utils.py 94.25% <100%> (+0.35%) ⬆️

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 00044f2...11e891c. Read the comment docs.

@victor-torres
Copy link
Contributor Author

Great. I have removed the changes from the requirements file in order to keep the scope of this PR attached to the original discussion.

@vshlapakov vshlapakov merged commit 74779f3 into scrapinghub:master Oct 27, 2018
@vshlapakov
Copy link
Contributor

Thanks @victor-torres!

@victor-torres victor-torres deleted the shub-jobauth branch October 29, 2018 20:06
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