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

idea: Restructure spidermon.contrib.scrapy.monitors #369

Closed
curita opened this issue Nov 24, 2022 · 2 comments · Fixed by #386
Closed

idea: Restructure spidermon.contrib.scrapy.monitors #369

curita opened this issue Nov 24, 2022 · 2 comments · Fixed by #386
Milestone

Comments

@curita
Copy link
Member

curita commented Nov 24, 2022

Background

spidermon/contrib/scrapy/monitors.py contains the following:

  • BaseScrapyMonitor
  • BaseStatMonitor
  • Builtin monitors that inherit from BaseStatMonitor or other mixins
  • Builtin monitor suites using some of those builtin monitors

Inconveniences

That module might contain many classes that could be split into submodules to organize them better.

Particularly it could be nice to have a submodule with all the monitors and another one with all the monitor suites, so that we can import only those in one go. This would be beneficial for Spidermon's docs, so we can import all the scrapy monitors at once instead of listing them individually, which can lead us to forget some from time to time (reference).

Proposal

spidermon/contrib/scrapy/monitors.py could be turned into a folder spidermon/contrib/scrapy/monitors/, with an __init__.pyfile that makes available everything that monitors.py has access to at the moment for backward compatibility.

spidermon/contrib/scrapy/monitors/ can have submodules for some specific kind of objects, for example:

  • ./base.py-> For BaseScrapyMonitor and BaseStatMonitor
  • ./monitors.py -> For the other scrapy builtin monitors
  • ./suites.py -> For the monitor suites

Let me know what you think!

@rennerocha
Copy link
Collaborator

It looks an excellent improvement. monitors.py has 583 lines (which is too big). Just ensuring that we don't have backwards incompatibility problems and existing code still work, I am in favor of that!

@rennerocha rennerocha added this to the 1.18.0 milestone Dec 15, 2022
@kennyaires
Copy link
Contributor

Hello all,

Happy to work on this, I'll take it as my new Rock for Q1 23.

@kennyaires kennyaires linked a pull request Feb 8, 2023 that will close this issue
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 a pull request may close this issue.

3 participants