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

Removed the ability to run the -O and -t command line option together (#5516) #5605

Merged
merged 6 commits into from
Oct 27, 2022

Conversation

MagnusOffermanns
Copy link
Contributor

Fix for Issue 5516

If -t and -O are used together scrapy now throws a usageError. I also created a error message including examples.

Furthermore, I added an extended better deprecation warning in case -t and -o
are used together including two examples.

Additionally I added a usageError message for the case that -t and -O
are used together including examples.

Futhermore added a extended better deprecation warning in case -t and -o
are used together including two examples.

 Changes to be committed:
	modified:   scrapy/utils/conf.py
@MagnusOffermanns
Copy link
Contributor Author

I have two inputs that I would like to get feedback on, as I do not really know how to handle the two cases. I would be very glad for your feedback.

  1. The warnings and error messages I created are all in one line similar to the already existing ones. Would it not make sense to make them a little bit more readable by giving them new lines? Is it maybe wanted that they only have one line in order to display them better?
  2. When the combination of -t and -O throws an error would it not also make sense to throw an error when -t and -o are used? Therefore making the -t option obsolete? How long do we have to keep -t till we can remove it from the source code?

I thank you for answering my questions and hope I could resolve this issue ok 🚀

 extended the help message.

Changes to be committed:
	modified:   docs/topics/commands.rst
	modified:   scrapy/commands/__init__.py
@MagnusOffermanns
Copy link
Contributor Author

I also extended the documentation for crawl and the help message. Maybe I also closed issue #5530.

With one caviat:
I was not able to make the -a command run. Therefore I did not add an example there. If someone gives me a tip how to use the -a option I will quickly add it as well.

Anyway, thank you for your help and feedback.

Copy link
Member

@wRAR wRAR 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 but there are some typos, can you please fix those?

@MagnusOffermanns MagnusOffermanns requested a review from wRAR October 7, 2022 20:15
@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #5605 (65bc100) into master (9fbd819) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 65bc100 differs from pull request most recent head 440d383. Consider uploading reports for the commit 440d383 to get more accurate results

@@            Coverage Diff             @@
##           master    #5605      +/-   ##
==========================================
- Coverage   88.67%   88.63%   -0.04%     
==========================================
  Files         162      162              
  Lines       10998    11000       +2     
  Branches     1801     1802       +1     
==========================================
- Hits         9752     9750       -2     
- Misses        966      968       +2     
- Partials      280      282       +2     
Impacted Files Coverage Δ
scrapy/commands/__init__.py 73.25% <ø> (ø)
scrapy/utils/conf.py 92.10% <0.00%> (-1.65%) ⬇️
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️

@Gallaecio Gallaecio merged commit fd692f3 into scrapy:master Oct 27, 2022
@wRAR wRAR linked an issue Nov 1, 2022 that may be closed by this pull request
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.

Feed is not ovewritten when custom extension is used
3 participants