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

Warn BaseSpider deprecation only once #519

Merged
merged 8 commits into from Jan 16, 2014
Merged

Warn BaseSpider deprecation only once #519

merged 8 commits into from Jan 16, 2014

Conversation

@dangra
Copy link
Member

@dangra dangra commented Jan 8, 2014

No description provided.

@@ -65,7 +65,7 @@ def __str__(self):
__repr__ = __str__


BaseSpider = create_deprecated_class('BaseSpider', Spider)
BaseSpider = create_deprecated_class('BaseSpider', Spider, warn_once=True)

This comment has been minimized.

@pablohoffman

pablohoffman Jan 8, 2014
Member

Why not warn once by default? (ie. warn_multiple=False kwarg). Looks like the most sensible behavior to me, so I would make it the default.

Alternatively, we could consider supporting the "action" field of warnings filter:
http://docs.python.org/2/library/warnings.html#the-warnings-filter

@dangra
Copy link
Member Author

@dangra dangra commented Jan 8, 2014

/cc @kmike

@kmike
Copy link
Member

@kmike kmike commented Jan 9, 2014

I agree that the original idea of showing a warning for each subclass is not great - that's not how warnings usually work. But I think that create_deprecated_class is a wrong place to control how much warnings to show - what if user wants all these warnings (to find all places the code needs to be updated)?

Maybe, as @pablohoffman said, we can use http://docs.python.org/2/library/warnings.html#warnings.filterwarnings instead?

Also, I think it is fine to show multiple warnings on instantiation - by default Python suppresses warnings only for duplicate lineno/module. It won't be annoying for scrapy list because only single spider is instantiated there. It will be annoying in tests as it should.

@dangra
Copy link
Member Author

@dangra dangra commented Jan 9, 2014

I get the functionality of warnings filters but I don't follow what is the idea on how to use them here, is the user supposed to configure them in their scrapy projects or should Scrapy set filters by default?

@dangra
Copy link
Member Author

@dangra dangra commented Jan 9, 2014

Also, I think it is fine to show multiple warnings on instantiation - by default Python suppresses warnings only for duplicate lineno/module. It won't be annoying for scrapy list because only single spider is instantiated there. It will be annoying in tests as it should.

we have to keep in mind that create_deprecated_class is going to be used for XPathItemLoader and XPathSelector classes too, it's common to instantiate those classes multiples times.

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Jan 9, 2014

Spider or BaseSpider is instantiated several times no matter the command you use, if it uses SpiderManager the spiders are going to be instantiated.

@dangra
Copy link
Member Author

@dangra dangra commented Jan 9, 2014

Spider or BaseSpider is instantiated several times no matter the command you use, if it uses SpiderManager the spiders are going to be instantiated.

@nramirezuy: SpiderManager loads all spider classes but it doesn't instantiate them. It's very rare to have mroe than one spider instantiate.

@kmike
Copy link
Member

@kmike kmike commented Jan 9, 2014

@dangra I think we may add a default filter which would show only the first warning; users can insert their own filters to override the default.

@kmike
Copy link
Member

@kmike kmike commented Jan 9, 2014

What's wrong with multiple warnings about XPathItemLoader? As far as I know, it won't be a warning per instantiation, it'd be a warning per instantiation location, the same behavior as now.

@dangra
Copy link
Member Author

@dangra dangra commented Jan 9, 2014

@kmike: I don't mind if you submit a PR to show how filterwarnings should work :)

@dangra
Copy link
Member Author

@dangra dangra commented Jan 9, 2014

What's wrong with multiple warnings about XPathItemLoader? As far as I know, it won't be a warning per instantiation, it'd be a warning per instantiation location, the same behavior as now.

you right, I messed it.

@dangra
Copy link
Member Author

@dangra dangra commented Jan 10, 2014

I defaulted warn_once to True, and reverted the changes made to instantation warnings. Also fixed a bug on subclassing warnings showing for sub-sub-classes instead of only direct subclasses.

this is not using warnings.filterwarnings at all.

@dangra
Copy link
Member Author

@dangra dangra commented Jan 13, 2014

Can anyone shed a light for what is going on here?

Warnings tests for deprecated-class-instantation are failing due to a weird issue with trial. Testcase passes only when the test runner (trial) doesn't load any tests inheriting from twisted.trial.unittest.TestCase.

I had been fiddling with warning filters pre/post trial import and within catch_warnings call, and although they are different, altering them doesn't make a difference unless I set a explicit 'once' filter.

Simple snippet to reproduce the issue:

import warnings                                                                                     
import unittest                                                                                     
import twisted.trial.unittest                                                                       


class Foo(twisted.trial.unittest.TestCase):                                                         

    def test_ignoreme(self):                                                                        
        pass                                                                                        


class Test(unittest.TestCase):                                                                      

    def test_foo(self):                                                                             
        with warnings.catch_warnings(record=True) as w:                                             
            for _ in range(3):                                                                      
                warnings.warn('no me jodas mas')                                                    

        self.assertEqual(len(w), 1)

running with trial fails:

warnings-differences
  Foo
    test_ignoreme ...                                                      [OK]
  Test
    test_foo ...                                                         [FAIL]

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 332, in run
    testMethod()
  File "/home/daniel/warnings-differences.py", line 19, in test_foo
    self.assertEqual(len(w), 1)
  File "/usr/lib/python2.7/unittest/case.py", line 516, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 509, in _baseAssertEqual
    raise self.failureException(msg)
exceptions.AssertionError: 3 != 1

warnings-differences.Test.test_foo
-------------------------------------------------------------------------------
Ran 2 tests in 0.021s

FAILED (failures=1, successes=1

but it passes if testcases inheriting from twisted.trial.unittest.TestCase are removed:

$ trial warnings-differences.py 
warnings-differences
  Test
    test_foo ...                                                           [OK]

-------------------------------------------------------------------------------
Ran 1 tests in 0.018s

PASSED (successes=1)
@kmike
Copy link
Member

@kmike kmike commented Jan 16, 2014

Let's merge it! I'll submit an another PR with warnings.filterwarnings if I ever get to it.

dangra added a commit that referenced this pull request Jan 16, 2014
Warn BaseSpider deprecation only once
@dangra dangra merged commit 5a175ad into scrapy:master Jan 16, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@dangra dangra deleted the dangra:warn-once branch Jan 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.