-
Notifications
You must be signed in to change notification settings - Fork 98
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
docs: Mixins documentation #133
Conversation
Issue #84 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gcmurillo Thanks for the pull request. Mixin sure seems interesting, can you list out sources from where you referred this information from.
A suggestion would be to link the Mixins
to their code present in /spidermon/contrib/monitors/mixins/
for reference purpose. As you are explaining each of them in the documentation. These little things helps the user to jump quickly back and forth if they ever need it.
Well, there is not sources where I can get information for this objects. I did a description with a general purpose review for each one. I will get your suggestion and put the code in the docs too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this documentation is very necessary, so thanks for working on this!
I have some feedback for you to consider:
-
Introductions don’t need a header, you could remove the first header completely.
-
I would use some word wrapping, keeping line length under Spidermon’s length limit for code (88 characters, Black’s) where possible.
-
I think the introduction can be summarized even further without loosing any meaningful information. For example:
A mixin is a class that implements one or more features that you can apply to any class through multiple inheritance. For more information, see `What is a mixin, and why are they useful?`_ .. _What is a mixin, and why are they useful?: https://stackoverflow.com/q/533631
I think one external link is enough. That StackOverflow question should always show
the most upvoted answers, so if a better answer comes up in the future, we don’t
need to update the documentation. -
You can use more than those 4 mixins with Spidermon, those are simply the mixins that Spidermon currently provides built-in, so I would rephrase the paragraph that introduces the API documentation a bit. For example:
Spidermon offers the following built-in mixins: `JobMonitorMixin`_, `SpiderMonitorMixin`_, `StatsMonitorMixin`_, `ValidationMonitorMixin`_.
Notice that I also skipped the number of them. It’s unnecessary, and easy to forget to update when new mixins are added in the future.
I also think we should keep the order alphabetical, both here and below on the actual API reference documentation, for easier lookup.
-
Because here we are documenting classes, I would document them using Sphinx‘s
.. class::
directive. In fact, I would seriously consider using the.. autoclass::
directive instead and documenting the classes in the source code instead. With these, you can take the opportunity to include the import path of these classes. -
I would remove the links to the source code. They link to the latest code on Github, which is slightly problematic, because in the future it can result on broken documentation of previous Spidermon versions. Instead, if you use the Sphinx directives for API definition, you can configure Sphinx to provide links to source code automatically, and these links point to the actual code that the documentation covers, embedded into the documentation with syntax highlighting.
-
I think the documentation of
StatsMonitorMixin
is too complex for such a simple mixin, which simply exposesself.data.stats
asself.stats
, or disables the monitor ifself.data.stats
is undefined. I think a single-paragraph description should be enough. -
Regarding
JobMonitorMixin
, I would avoid describing it simply as ‘being similar to something else’, and provide a regular description. -
In
SpiderMonitorMixin
, I would try to be a bit more specific about what the mixin offers in addition to the two mixins it inherits: Which property of the original class is exposed as what? When is a monitor with this mixin disabled?Also, I would not describe private members in the documentation (
self._responses
). Describe the public ones instead (self.responses
).And it might be a good chance to document the
ResponsesInfo
class as well, also using Sphinx’sclass
directive, and linking to it. -
The documentation of
ValidationMonitorMixin
looks poor, and covers a private member only. Instead, it should cover the public counterpart of that private member, as well as the non-private methods that the mixing provides, documented with the corresponding Sphinx directive (.. method::
or.. automethod::
).
@gcmurillo do you plan to continue working on this issue? |
As the last activity in this PR was in May, I am closing it in favor of #232 |
Adding docs for mixin objects in
spidermincontrib/monitors/mixins