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

Update contributing guidelines to include pre-commit install #392

Merged
merged 4 commits into from Mar 3, 2023

Conversation

VMRuiz
Copy link
Collaborator

@VMRuiz VMRuiz commented Mar 3, 2023

No description provided.

@VMRuiz VMRuiz requested review from wRAR and Gallaecio March 3, 2023 07:44
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a641cbf) 76.41% compared to head (0252825) 76.41%.

❗ Current head 0252825 differs from pull request most recent head e193964. Consider uploading reports for the commit e193964 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #392   +/-   ##
=======================================
  Coverage   76.41%   76.41%           
=======================================
  Files          76       76           
  Lines        3193     3193           
  Branches      378      378           
=======================================
  Hits         2440     2440           
  Misses        683      683           
  Partials       70       70           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@VMRuiz VMRuiz requested a review from curita March 3, 2023 08:07
CONTRIBUTING.rst Outdated Show resolved Hide resolved
itemadapter
pre-commit
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how requirements.txt are organized, but probably it'd be better to make an explicit requirement-dev.txt file, as not all of these requirements are needed to run spidermon.

Also, it seems some of the requirements which are here are not in setup.py, e.g. itemadapter is not in setup.py install_requires (it's in test_requires for some reason).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the package requirements are weirdly structured, we could open a new issue to fix that

Copy link
Member

@kmike kmike Mar 3, 2023

Choose a reason for hiding this comment

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

Makes sense; we don't need to fix it in this PR.

Not having itemadapter in install_requires looks like a separate issue, I think it's a bug (no need to fix it in this PR either).

Copy link
Member

@kmike kmike left a comment

Choose a reason for hiding this comment

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

👍

@VMRuiz VMRuiz merged commit 558aace into scrapinghub:master Mar 3, 2023
@VMRuiz VMRuiz added this to the 1.18.0 milestone Mar 3, 2023
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

2 participants