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

docs: use __init__ method instead of constructor #4088

Merged
merged 6 commits into from Nov 12, 2019

Conversation

ammarnajjar
Copy link
Contributor

@ammarnajjar ammarnajjar commented Oct 21, 2019

Resolves #4086

@codecov
Copy link

@codecov codecov bot commented Oct 21, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4088      +/-   ##
==========================================
- Coverage   85.68%   83.04%   -2.65%     
==========================================
  Files         165      165              
  Lines        9734     9734              
  Branches     1463     1463              
==========================================
- Hits         8341     8084     -257     
- Misses       1136     1385     +249     
- Partials      257      265       +8
Impacted Files Coverage Δ
scrapy/utils/python.py 78.26% <ø> (-5.98%) ⬇️
scrapy/exporters.py 99.13% <ø> (-0.87%) ⬇️
scrapy/utils/datatypes.py 54.45% <ø> (-5.76%) ⬇️
scrapy/extensions/feedexport.py 81.65% <ø> (ø) ⬆️
scrapy/linkextractors/sgml.py 0% <0%> (-96.81%) ⬇️
scrapy/linkextractors/regex.py 0% <0%> (-95.66%) ⬇️
scrapy/linkextractors/htmlparser.py 0% <0%> (-92.07%) ⬇️
scrapy/extensions/statsmailer.py 0% <0%> (-30.44%) ⬇️
scrapy/robotstxt.py 73.68% <0%> (-23.69%) ⬇️
scrapy/_monkeypatches.py 54.54% <0%> (-18.19%) ⬇️
... and 18 more

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 21, 2019

You have successfully used `` in many documentation changes, but some changes use a single ` instead, which is incorrect (it applies italics).

@ammarnajjar ammarnajjar force-pushed the 4086-constructor-initializer-docs branch from 9ef093a to ecb6cfd Compare Oct 21, 2019
@ammarnajjar ammarnajjar force-pushed the 4086-constructor-initializer-docs branch from ecb6cfd to da8cd94 Compare Oct 21, 2019
@ammarnajjar
Copy link
Contributor Author

@ammarnajjar ammarnajjar commented Oct 21, 2019

thanks for the review, I hope I got it right this time 😅

@@ -265,7 +265,7 @@ There are several ways to modify Item Loader context values:
loader.context['unit'] = 'cm'

2. On Item Loader instantiation (the keyword arguments of Item Loader
constructor are stored in the Item Loader context)::
``__init__`` methodare stored in the Item Loader context)::
Copy link
Member

@Gallaecio Gallaecio Oct 22, 2019

Choose a reason for hiding this comment

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

Suggested change
``__init__`` methodare stored in the Item Loader context)::
``__init__`` method are stored in the Item Loader context)::

I’ve seen this issue in a few more replacements below, where the character right after ‘method’ (a space, a dot, a comma) was accidentally removed. Could you please take a look?

Copy link
Contributor Author

@ammarnajjar ammarnajjar Oct 22, 2019

Choose a reason for hiding this comment

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

🤦‍♂ of course

Copy link
Contributor Author

@ammarnajjar ammarnajjar Oct 22, 2019

Choose a reason for hiding this comment

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

@Gallaecio thanks for your patience, change pushed.

@ammarnajjar ammarnajjar force-pushed the 4086-constructor-initializer-docs branch from 5219435 to d21e103 Compare Oct 22, 2019
Copy link
Member

@Gallaecio Gallaecio left a comment

Thank you for taking your time, these consistency changes can be tiresome but are important to avoid user confusion.

There’s only one more thing I would like to bring up: some comments used ‘constructor’ to refer to methods other than __init__ used to create instances (e.g. from_crawler, from_settings). I believe in those cases the right wording would be “factory method”.

That said, I’m OK with merging the changes as is, I believe they are a significant improvement.

# 1. with no alternative __init__ methods
# 2. with from_settings() __init__ method
# 3. with from_crawler() __init__ method
# 4. with from_settings() and from_crawler() __init__ method
Copy link
Member

@kmike kmike Oct 22, 2019

Choose a reason for hiding this comment

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

I think here "constructor" is replaced with __init__ incorrectly here; "constructor" here is a single word for from_crawler/from_settings/etc., not as a synonym to __init__.

Copy link
Contributor Author

@ammarnajjar ammarnajjar Oct 22, 2019

Choose a reason for hiding this comment

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

so should one leave it as is "constructor" or use “factory method” instead as @Gallaecio mentioned in #4088 (review)?

Copy link
Member

@kmike kmike Oct 22, 2019

Choose a reason for hiding this comment

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

Yeah, I think so; no strong opinion about constructor/factory methods, I was always thinking about them as of "alternative constructors", so comment before the change as-is looks fine to me.

Copy link
Contributor Author

@ammarnajjar ammarnajjar Oct 22, 2019

Choose a reason for hiding this comment

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

@kmike thanks for the review, 7a84a4b is pushed.

@@ -106,7 +106,7 @@ def __init__(self, uri, access_key=None, secret_key=None, acl=None):
warnings.warn(
"Initialising `scrapy.extensions.feedexport.S3FeedStorage` "
"without AWS keys is deprecated. Please supply credentials or "
"use the `from_crawler()` constructor.",
"use the `from_crawler()` ``__init__`` method.",
Copy link
Member

@kmike kmike Oct 22, 2019

Choose a reason for hiding this comment

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

it should be kept as "constructor" here

Copy link
Contributor Author

@ammarnajjar ammarnajjar Oct 22, 2019

Choose a reason for hiding this comment

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

when I thought I got them all... bf5c1a3

sep/sep-009.rst Outdated Show resolved Hide resolved
ammarnajjar and others added 2 commits Oct 22, 2019
Issue scrapy#4086

Co-Authored-By: Mikhail Korobov <kmike84@gmail.com>
wRAR
wRAR approved these changes Nov 12, 2019
@wRAR wRAR merged commit c911e80 into scrapy:master Nov 12, 2019
1 check passed
@ammarnajjar ammarnajjar deleted the 4086-constructor-initializer-docs branch Nov 13, 2019
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.

4 participants