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] Update project.py removed one 'hack', seems irrelevant. #3910

Merged
merged 8 commits into from Aug 8, 2019
Merged

[MRG+1] Update project.py removed one 'hack', seems irrelevant. #3910

merged 8 commits into from Aug 8, 2019

Conversation

sbs2001
Copy link
Contributor

@sbs2001 sbs2001 commented Jul 28, 2019

As mentioned by @Gallaecio in issue #3871, the 'hack' is cleared. I also double checked whether the environment variable "SCRAPY_PICKLED_SETTINGS_TO_OVERRIDE" was ever set in our codebase and it turns out we didn't set it or used it anywhere else.So I guess the 'hack' was not used in the current version.

As mentioned by @Gallaecio in issue #3871, the 'hack'  is cleared. I also  double checked whether the environment variable "SCRAPY_PICKLED_SETTINGS_TO_OVERRIDE" was ever set in our codebase and it turns out we didn't set it or used it anywhere else.So I guess the 'hack' was not used in the current version. Also the name of this environment variable rather doesn't suggest it was  a boolean(it is used in  an 'if' condition which has perplexed me )
@@ -68,10 +68,7 @@ def get_project_settings():
settings.setmodule(settings_module_path, priority='project')

# XXX: remove this hack
Copy link
Member

@noviluni noviluni Jul 29, 2019

Choose a reason for hiding this comment

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

I think that # XXX: remove this hack should be removed as well.

Copy link
Contributor Author

@sbs2001 sbs2001 Jul 29, 2019

Choose a reason for hiding this comment

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

The code actually has 2 hacks,so thought it won't matter. Anyway I am doing what you suggested. Thank you.

@codecov
Copy link

@codecov codecov bot commented Jul 29, 2019

Codecov Report

Merging #3910 into master will decrease coverage by 0.57%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3910      +/-   ##
==========================================
- Coverage   85.47%   84.89%   -0.58%     
==========================================
  Files         165      165              
  Lines        9624     9621       -3     
  Branches     1446     1445       -1     
==========================================
- Hits         8226     8168      -58     
- Misses       1145     1192      +47     
- Partials      253      261       +8
Impacted Files Coverage Δ
scrapy/utils/project.py 78.84% <ø> (+2.48%) ⬆️
scrapy/core/downloader/handlers/s3.py 62.9% <0%> (-32.26%) ⬇️
scrapy/utils/boto.py 46.66% <0%> (-26.67%) ⬇️
scrapy/core/downloader/tls.py 75.92% <0%> (-12.97%) ⬇️
scrapy/utils/ssl.py 51.42% <0%> (-5.72%) ⬇️
scrapy/extensions/feedexport.py 78.44% <0%> (-5.05%) ⬇️
scrapy/core/downloader/handlers/http11.py 89.92% <0%> (-2.62%) ⬇️
scrapy/core/scraper.py 86.48% <0%> (-2.03%) ⬇️
scrapy/pipelines/files.py 65.38% <0%> (-1.16%) ⬇️

@codecov
Copy link

@codecov codecov bot commented Jul 29, 2019

Codecov Report

Merging #3910 into master will not change coverage.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master    #3910   +/-   ##
=======================================
  Coverage   85.47%   85.47%           
=======================================
  Files         165      165           
  Lines        9624     9624           
  Branches     1446     1446           
=======================================
  Hits         8226     8226           
  Misses       1145     1145           
  Partials      253      253
Impacted Files Coverage Δ
scrapy/utils/project.py 75.43% <50%> (-0.93%) ⬇️
scrapy/utils/ssl.py 52.77% <0%> (-4.37%) ⬇️
scrapy/pipelines/files.py 66.53% <0%> (ø) ⬆️
scrapy/_monkeypatches.py 72.72% <0%> (+15.58%) ⬆️

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Jul 30, 2019

I think we should to first deprecate the variable, logging a deprecation warning when it’s defined, and actually drop it at a latter version.

@sbs2001
Copy link
Contributor Author

@sbs2001 sbs2001 commented Jul 30, 2019

I would like to handle that. I have currently implemented a warning instead of dropping the whole thing.Please review and suggest changes if any.

How about this?
scrapy/utils/project.py Outdated Show resolved Hide resolved
scrapy/utils/project.py Outdated Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio left a comment

I’ve left a couple of additional nitpicks, but I think this can be merged as is. Thanks!

scrapy/utils/project.py Outdated Show resolved Hide resolved
scrapy/utils/project.py Outdated Show resolved Hide resolved
@Gallaecio Gallaecio changed the title Update project.py removed one 'hack', seems irrelevant. [MRG+1] Update project.py removed one 'hack', seems irrelevant. Aug 1, 2019
sbs2001 and others added 2 commits Aug 1, 2019
Co-Authored-By: Adrián Chaves <adrian@chaves.io>
Co-Authored-By: Adrián Chaves <adrian@chaves.io>
@@ -1,6 +1,7 @@
import os
from six.moves import cPickle as pickle
import warnings
from scrapy.exceptions import ScrapyDeprecationWarning
Copy link
Member

@kmike kmike Aug 1, 2019

Choose a reason for hiding this comment

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

one more nitpick: could you please move this import to another scrapy.exceptions import below? According to pep8, imports should be grouped in 3 groups: stdlib imports, third-party imports, project imports; this one should be in the last group.

Copy link
Contributor Author

@sbs2001 sbs2001 Aug 1, 2019

Choose a reason for hiding this comment

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

@kmike missed that one thanks for the suggestion. I have made the changes accordingly.

@kmike
Copy link
Member

@kmike kmike commented Aug 8, 2019

Thanks @sbs2001 for a fix, and everyone else for feedback!

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

4 participants