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

[MRG+1] Add SCRAPY_CHECK environment variable #3739

Merged
merged 10 commits into from Jun 13, 2019

Conversation

@Matthijsy
Copy link
Contributor

@Matthijsy Matthijsy commented Apr 10, 2019

This PR implements the environment variable when running scrapy check as asked in #3704 . This way it is possible to have different behaviour in the scraper when running the check (like requiring less settings to be set)

fixes #3704

@codecov
Copy link

@codecov codecov bot commented Apr 10, 2019

Codecov Report

Merging #3739 into master will increase coverage by <.01%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #3739      +/-   ##
==========================================
+ Coverage   85.42%   85.43%   +<.01%     
==========================================
  Files         169      169              
  Lines        9635     9647      +12     
  Branches     1433     1436       +3     
==========================================
+ Hits         8231     8242      +11     
- Misses       1156     1157       +1     
  Partials      248      248
Impacted Files Coverage Δ
scrapy/utils/misc.py 96.29% <100%> (+0.58%) ⬆️
scrapy/commands/check.py 25.35% <9.09%> (-0.37%) ⬇️

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Apr 17, 2019

Could you please add an automated test to cover this change?

@Matthijsy Matthijsy closed this Apr 18, 2019
@Matthijsy Matthijsy reopened this Apr 18, 2019
@Matthijsy
Copy link
Contributor Author

@Matthijsy Matthijsy commented Apr 20, 2019

I've added a test for the set_environ util. I couldn't find the test of scrapy check so don't know how I should write a test for that part. Is this untested or can you tell me where I can find it?

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Apr 26, 2019

scrapy/tests/test_cmdline/__init__.py tests some commands, but there are no tests for scrapy check currently, no, so I’m OK with your test coverage.

It would be great if you could cover the new environment variable in the documentation about scrapy check. Other than that, I think this is ready to merge.

@Matthijsy Matthijsy force-pushed the feature/scrapy_check_env branch from 16f7ca5 to 1b3763d Apr 28, 2019
@Matthijsy
Copy link
Contributor Author

@Matthijsy Matthijsy commented May 2, 2019

@Gallaecio Thanks for your feedback! I've added the documentation, hope this is the right way to document it and otherwise I would love to update the documentation.

Copy link
Member

@Gallaecio Gallaecio left a comment

Please, build the documentation locally to see how your changes look.

To build the documentation, you can create a virtual environment with the dependencies from requirements-py3.txt and docs/requirements.txt, and run make html from the docs folder. You can then open docs/build/html/index.html in a web browser and reach the contracts page.

scrapy/utils/misc.py Show resolved Hide resolved
scrapy/utils/misc.py Outdated Show resolved Hide resolved
tests/test_utils_misc/__init__.py Outdated Show resolved Hide resolved
@Matthijsy Matthijsy force-pushed the feature/scrapy_check_env branch from 1b3763d to 8bd207a May 3, 2019
@Matthijsy Matthijsy force-pushed the feature/scrapy_check_env branch from a866b07 to f6485e6 May 3, 2019
@Matthijsy
Copy link
Contributor Author

@Matthijsy Matthijsy commented May 3, 2019

Thanks for the comment on how to build the documentation! I builded it and think it looks fine now

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented May 3, 2019

I’ve refactored your documentation a bit. Please, review my changes.

@Gallaecio Gallaecio changed the title Add SCRAPY_CHECK environment variable [MRG+1] Add SCRAPY_CHECK environment variable May 3, 2019
@Matthijsy
Copy link
Contributor Author

@Matthijsy Matthijsy commented May 3, 2019

Great! Looks fine to me

@dangra dangra merged commit cdfb20a into scrapy:master Jun 13, 2019
2 of 3 checks passed
@dangra dangra added this to the v1.7 milestone Jun 13, 2019
@Matthijsy Matthijsy deleted the feature/scrapy_check_env branch Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants