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

Fix TypeError in parse command #5482

Merged
merged 4 commits into from Apr 29, 2022
Merged

Fix TypeError in parse command #5482

merged 4 commits into from Apr 29, 2022

Conversation

alexpdev
Copy link
Contributor

Fixes #5481

scrapy parse -h now displays the appropriate help message.

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #5482 (413dd33) into 2.6 (aead27b) will decrease coverage by 0.46%.
The diff coverage is n/a.

❗ Current head 413dd33 differs from pull request most recent head 915c288. Consider uploading reports for the commit 915c288 to get more accurate results

@@            Coverage Diff             @@
##              2.6    #5482      +/-   ##
==========================================
- Coverage   88.77%   88.31%   -0.47%     
==========================================
  Files         163      163              
  Lines       10676    10554     -122     
  Branches     1821     1786      -35     
==========================================
- Hits         9478     9321     -157     
- Misses        922      953      +31     
- Partials      276      280       +4     
Impacted Files Coverage Δ
scrapy/commands/parse.py 27.68% <ø> (+0.71%) ⬆️
scrapy/robotstxt.py 73.33% <0.00%> (-24.20%) ⬇️
scrapy/core/downloader/handlers/__init__.py 83.33% <0.00%> (-9.40%) ⬇️
scrapy/squeues.py 92.85% <0.00%> (-7.15%) ⬇️
scrapy/utils/ssl.py 65.85% <0.00%> (-4.88%) ⬇️
scrapy/pqueues.py 95.86% <0.00%> (-3.31%) ⬇️
scrapy/extensions/memdebug.py 45.00% <0.00%> (-2.62%) ⬇️
scrapy/downloadermiddlewares/cookies.py 95.74% <0.00%> (-2.16%) ⬇️
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️
scrapy/extensions/debug.py 44.73% <0.00%> (-1.42%) ⬇️
... and 64 more

scrapy/commands/parse.py Outdated Show resolved Hide resolved
@elacuesta elacuesta changed the title Fixes Issue #5481 Fix TypeError in parse command Apr 21, 2022
Copy link
Member

@elacuesta elacuesta left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. Two suggestions for the future:

  • There's no need to open an issue if you already have a patch in mind, you can submit a PR directly
  • Use a descriptive name in the PR. This will (probably) be the message we end up using when merging, and "fixes issue #N" is not good for the history

@alexpdev
Copy link
Contributor Author

Looks good, thank you. Two suggestions for the future:

  • There's no need to open an issue if you already have a patch in mind, you can submit a PR directly
  • Use a descriptive name in the PR. This will (probably) be the message we end up using when merging, and "fixes issue #N" is not good for the history

Thanks for the advice.

@Gallaecio
Copy link
Member

@Laerte Could you rebase this change into the 2.6 branch instead of master?

@Laerte
Copy link
Member

Laerte commented Apr 22, 2022

@Gallaecio You want me to cherry-pick the changes from his branch and apply on-top 2.6 branch? In this case we will need open another PR? (I don't have write access to his branch that's why i thinked on this approach).

@alexpdev
Copy link
Contributor Author

How do i grant write access?

@Laerte
Copy link
Member

Laerte commented Apr 22, 2022

@alexpdev You can follow these steps: #5445 (comment)

(Change the commit id of course)

@alexpdev
Copy link
Contributor Author

alexpdev commented Apr 22, 2022

@alexpdev You can follow these steps: #5445 (comment)

@Laerte okay.. i figured it out. I think
Do I submit a pr?

@Laerte
Copy link
Member

Laerte commented Apr 23, 2022

@alexpdev Just push and change the base here to 2.6.

@alexpdev
Copy link
Contributor Author

Did I do that right?

@Laerte
Copy link
Member

Laerte commented Apr 24, 2022

@alexpdev Yeah, now you just have to change this to 2.6 as well:

Screenshot_20220424-031118-346

@alexpdev
Copy link
Contributor Author

got it

@alexpdev alexpdev changed the base branch from master to 2.6 April 24, 2022 19:20
@Laerte
Copy link
Member

Laerte commented Apr 24, 2022

@alexpdev Almost there... As you can see we have commits from master in the commits list, you need to drop them and let just yours commits. You can do this:

  1. Rebase from 636127e commit.
git rebase -i 636127e^
  1. This will open a text-editor, you need to drop all the commits that are not yours, you can archive this replacing pick to drop.

I recorded my terminal: https://asciinema.org/a/lbF5Pv8v8cNtkfTYeudPTNVO8

Then you need to push again:

git push -f

@alexpdev
Copy link
Contributor Author

@Laerte Okay, got it... thanks again for all of your help

@Laerte
Copy link
Member

Laerte commented Apr 24, 2022

Nice job! 🚀

@wRAR wRAR added this to the 2.6.2 milestone Apr 28, 2022
@wRAR wRAR merged commit b9b9422 into scrapy:2.6 Apr 29, 2022
@wRAR
Copy link
Member

wRAR commented Apr 29, 2022

Thank you!

@alexpdev alexpdev deleted the parse_help_msg branch June 14, 2022 21:12
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.

scrapy parse -h throws error
5 participants