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

Remove assertions from production code #4440

Merged
merged 3 commits into from Apr 23, 2020

Conversation

elacuesta
Copy link
Member

@elacuesta elacuesta commented Mar 18, 2020

Inspired by the following comment by @Gallaecio: #4054 (comment)

From the docs about the assert statement:

The current code generator emits no code for an assert statement when optimization is requested at compile time

Please pay attention to the specific exception classes, there might be better ones in some cases.

@elacuesta elacuesta force-pushed the remove-assertions-outside-of-tests branch from 4aadbff to 46befb8 Compare Mar 18, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 18, 2020

Codecov Report

Merging #4440 into master will decrease coverage by 0.25%.
The diff coverage is 11.76%.

@@            Coverage Diff             @@
##           master    #4440      +/-   ##
==========================================
- Coverage   84.78%   84.53%   -0.26%     
==========================================
  Files         164      164              
  Lines        9888     9905      +17     
  Branches     1464     1480      +16     
==========================================
- Hits         8384     8373      -11     
- Misses       1248     1263      +15     
- Partials      256      269      +13     
Impacted Files Coverage Δ
scrapy/commands/__init__.py 61.11% <0.00%> (-1.16%) ⬇️
scrapy/contracts/default.py 84.31% <0.00%> (-3.69%) ⬇️
scrapy/core/engine.py 87.87% <0.00%> (-5.02%) ⬇️
scrapy/core/scraper.py 87.97% <0.00%> (-1.20%) ⬇️
scrapy/crawler.py 88.26% <0.00%> (-1.06%) ⬇️
scrapy/http/request/__init__.py 97.26% <0.00%> (-2.74%) ⬇️
scrapy/pipelines/files.py 60.92% <0.00%> (-0.74%) ⬇️
scrapy/utils/reactor.py 80.00% <0.00%> (-3.34%) ⬇️
scrapy/core/downloader/middleware.py 96.36% <33.33%> (-3.64%) ⬇️
scrapy/utils/iterators.py 97.75% <100.00%> (+0.05%) ⬆️

@elacuesta elacuesta changed the title Remove assertions in production code Remove assertions from production code Mar 18, 2020
scrapy/core/engine.py Outdated Show resolved Hide resolved
@elacuesta
Copy link
Member Author

@elacuesta elacuesta commented Mar 30, 2020

I wonder why there is no Travis build for the latest commit 🤔

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Mar 30, 2020

There is. No idea why it does not show up in the pull request any more, though.

@elacuesta elacuesta closed this Apr 1, 2020
@elacuesta elacuesta reopened this Apr 1, 2020
@elacuesta elacuesta force-pushed the remove-assertions-outside-of-tests branch from 1261f5a to a802936 Compare Apr 1, 2020
@kmike
Copy link
Member

@kmike kmike commented Apr 18, 2020

Exceptions makes sense, thanks @elacuesta!
I added "backwards-incompatible" tag, because technically this changes the behavior.

kmike
kmike approved these changes Apr 18, 2020
@Gallaecio Gallaecio merged commit efb6f13 into scrapy:master Apr 23, 2020
1 of 2 checks passed
@elacuesta elacuesta deleted the remove-assertions-outside-of-tests branch Apr 23, 2020
Gallaecio added a commit to Gallaecio/scrapy that referenced this issue Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants