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

allowing to run .pyw files #4646

Merged
merged 18 commits into from Oct 1, 2020
Merged

allowing to run .pyw files #4646

merged 18 commits into from Oct 1, 2020

Conversation

akshaysharmajs
Copy link
Contributor

@akshaysharmajs akshaysharmajs commented Jun 24, 2020

Referenced to issue #4643.
Also looking for other parts of code base for the mentioned issue.

Fixes #4643

scrapy/commands/runspider.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #4646 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4646      +/-   ##
==========================================
- Coverage   87.21%   87.19%   -0.03%     
==========================================
  Files         160      160              
  Lines        9813     9813              
  Branches     1447     1447              
==========================================
- Hits         8558     8556       -2     
- Misses        994      995       +1     
- Partials      261      262       +1     
Impacted Files Coverage Δ
scrapy/commands/runspider.py 89.13% <100.00%> (ø)
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Could you include a test?

@Gallaecio Gallaecio self-requested a review June 24, 2020 12:48
@akshaysharmajs
Copy link
Contributor Author

Could you include a test?

Okay, I will add some test.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 25, 2020

scrapy\scrapy\commands\edit.py:`
36
37 sfile = sys.modules[spidercls.module].file
38: sfile = sfile.replace('.pyc', '.py')
39 self.exitcode = os.system('%s "%s"' % (editor, sfile))
40
scrapy\scrapy\commands\genspider.py:
99 spiders_module = None
100 spiders_dir = "."
101: spider_file = "%s.py" % join(spiders_dir, module)
102 shutil.copyfile(template_file, spider_file)
103 render_templatefile(spider_file, **tvars)

scrapy\scrapy\commands\startproject.py:
23: IGNORE = ignore_patterns('*.pyc', '.svn')

I searched code base and these are the concerned results. I don't think they require .pyw?Please check

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 25, 2020

image

I m getting this error when runnig tox command and not able to test above changes. Tried installing 'itemadapter' again but error is still there.

@wRAR
Copy link
Member

wRAR commented Jun 26, 2020

@akshaysharmajs try removing your .tox dir, this helps when the project requirements have changed.

@akshaysharmajs
Copy link
Contributor Author

@akshaysharmajs try removing your .tox dir, this helps when the project requirements have change

Thanks! @wRAR

@akshaysharmajs
Copy link
Contributor Author

Should I make test for .pyw files windows specific that will only run in windows. I tried mocking windows but '.pyw' doesn't support in linux. giving error (No module named 'myspider.pyw')

@Gallaecio
Copy link
Member

Gallaecio commented Jun 29, 2020

I think we should refactor these tests.

I’m thinking we could have a base class that defines the tests and reads the filename (myspider.py or myspider.pyw) from a class variable, and then have 2 subclasses that simply define that variable, each with one of the values. See HttpProxyTestCase and its subclasses for an example of what I mean.

I would also have the tests that will also work out of Windows run in other OSes. Mostly because at the moment our CI does not run Windows tests. For the remaining tests, we can use skipif to skip them outside Windows.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Jun 29, 2020

Thanks, I can try this implementation.

@akshaysharmajs
Copy link
Contributor Author

Please check, I just used RunSpiderCommandTest as base class for .py and subclass WindowsRunSpiderCommandTest for '.pyw'.

tests/test_commands.py Outdated Show resolved Hide resolved
tests/test_commands.py Outdated Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio 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 to me, thanks!

PS: The test failure is unrelated.

@akshaysharmajs
Copy link
Contributor Author

akshaysharmajs commented Sep 2, 2020

Some more tests have been added that cause test failure.
Only these two are WindowsRunSpiderCommandTest.test_output and WindowsRunSpiderCommandTest.test_overwrite_output failing so I should skip them for unix platforms.

@kmike kmike merged commit 159e2b2 into scrapy:master Oct 1, 2020
@kmike
Copy link
Member

kmike commented Oct 1, 2020

Nice, thanks @akshaysharmajs!

@akshaysharmajs
Copy link
Contributor Author

😃 😃

@akshaysharmajs akshaysharmajs deleted the pywfiles branch December 25, 2020 18:23
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.

Allow run pyw scripts
4 participants