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

Allow passing classes directly in Settings #3873

Merged
merged 7 commits into from Aug 26, 2020

Conversation

nyov
Copy link
Contributor

@nyov nyov commented Jul 12, 2019

Implements #3870 (See MWE code over there):

  • Pass class objects directly in Settings(), instead of needing to go through Scrapy magic.

As @kmike kindly pointed out, this functionality was previously PR'd in some form in #1215, #1032, and as part of Addons.
However it never got done. Let's get this done?

@kmike
Copy link
Member

kmike commented Jul 12, 2019

The implementation looks good to me.

I think to make it in it'd be awesome to have tests and docs for the end-user feature - class objects in Settings.

@nyov
Copy link
Contributor Author

nyov commented Jul 12, 2019

Hm, I could write some docs. The Settings-related tests... are a boatload.
Tbh, I don't even know where I should add a case there and where not, as I don't grasp the whole Settings-functionalities since they got the Attributes and priorities layers.

I will have a think on that, and need to find some time to parse that code.

@kmike
Copy link
Member

kmike commented Jul 12, 2019

you may have a test similar to your example - create a custom middleware class, pass it in settings, check that it works

@nyov
Copy link
Contributor Author

nyov commented Jul 12, 2019

I tried. Sorry to disappoint, it seems that's not something I can manage in a reasonable amount of time.
Oh well, I found this: jdemaeyer@7fca7b5
Would these work?
Actually not
But do we need those MW-changes over there, also?

@Gallaecio Gallaecio closed this Jul 15, 2019
@Gallaecio Gallaecio reopened this Jul 15, 2019
@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #3873 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3873      +/-   ##
==========================================
+ Coverage   85.56%   85.56%   +<.01%     
==========================================
  Files         164      164              
  Lines        9551     9554       +3     
  Branches     1431     1432       +1     
==========================================
+ Hits         8172     8175       +3     
  Misses       1132     1132              
  Partials      247      247
Impacted Files Coverage Δ
scrapy/utils/deprecate.py 95.45% <100%> (+0.06%) ⬆️
scrapy/utils/misc.py 96.38% <100%> (+0.08%) ⬆️

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #3873 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3873      +/-   ##
==========================================
- Coverage   87.19%   87.17%   -0.02%     
==========================================
  Files         160      160              
  Lines        9815     9819       +4     
  Branches     1447     1449       +2     
==========================================
+ Hits         8558     8560       +2     
- Misses        995      996       +1     
- Partials      262      263       +1     
Impacted Files Coverage Δ
scrapy/utils/deprecate.py 95.38% <100.00%> (ø)
scrapy/utils/misc.py 96.09% <100.00%> (+0.12%) ⬆️
scrapy/utils/defer.py 93.47% <0.00%> (-2.18%) ⬇️

@Gallaecio
Copy link
Member

Gallaecio commented Jul 15, 2019

It may also be worth adding a test that checks that non-callables non-strings raise a TypeError.

@kmike
Copy link
Member

kmike commented Jul 15, 2019

But do we need those MW-changes over there, also?

@nyov I think these changes were to support object instances, not classes; I'd prever to leave them out for now.

@nyov
Copy link
Contributor Author

nyov commented Jul 21, 2019

Thanks for the hints!
I'm not sure if those tests now are suitable or good enough, probably not. But maybe you can give some more hints now that I did some work? :p
The paragraph in the docs also doesn't sound really awesome, but I didn't manage to make it more concise without sacrificing legibility for (likely non-native) readers. Improvements welcome.

edit: Let me squash those commits before you merge, in any case.

@nyov nyov changed the title Allow passing classes directly in Settings [WIP] Allow passing classes directly in Settings Jul 21, 2019
scrapy/utils/misc.py Show resolved Hide resolved
@elacuesta elacuesta added this to the v2.3 milestone Jul 9, 2020
@kmike kmike modified the milestones: v2.3, 2.4 Jul 16, 2020
@Gallaecio
Copy link
Member

Gallaecio commented Aug 20, 2020

I took the liberty to refactor the documentation:

  • Move it to a section of its own, so that the scope of versionadded is clearer
  • Refactor the first two paragraph so that they do not speak in terms of before and after (more fitting for the release notes), but rather describe the current situation.
  • Removed the inline examples, as the example below show suffice.
  • Removed the note about deprecation handling; it was not 100% accurate (deprecation checks should still work, and happen when those objects are used at run time; fixing the paths of removed imports is was will not be there, but I don’t think that needs to be described explicitly)

Copy link
Member

@Gallaecio Gallaecio left a comment

While I still think we could support anything but a string, I’m OK with only supporting callables for the time being.

@elacuesta elacuesta changed the title [WIP] Allow passing classes directly in Settings Allow passing classes directly in Settings Aug 20, 2020
@nyov
Copy link
Contributor Author

nyov commented Aug 23, 2020

@Gallaecio, many thanks for the work.
I'm sorry for dropping the ball on my scrapy contributions, but I don't currently have enough free time to continue them.
If you feel like finishing this PR, I totally wouldn't mind. But no pressure (from me).

scrapy/utils/misc.py Outdated Show resolved Hide resolved
kmike
kmike approved these changes Aug 26, 2020
Copy link
Member

@kmike kmike left a comment

Thanks @nyov and @Gallaecio, looks great! +1 to merge.

I've left a minor comment about docs; feel free to merge with or without addressing iit @Gallaecio.

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