Fix #3516 - Erroneous @classmethod on NoneQuery.match by adamjakab · Pull Request #3517 · beetbox/beets

adamjakab · 2020-03-15T11:55:17Z

It seems like tests are not hitting this method...

Checklist

Fix
Test
Changelog

adamjakab · 2020-03-15T14:02:56Z

The NoneQuery class is now 100% covered. I added a missing value_match abstract method which later I removed because I was unable to reach/test it. I am not sure how it works but I think in the case of the NoneQuery this method can not be reached. Or am I wrong?

sampsyo · 2020-03-15T16:00:08Z

Great; thanks!!

The value_match method is indeed unreachable; I think we should leave it off here. It is only used by the base implementation of match for cases where the behavior is best defined in terms of the pattern and value. Although the docstring says "Subclasses must provide a value_match class method," it should probably instead say that they should either provide value_match or just override match itself.

Looks like there's also a test failure related to the Python 3 behavior of repr on strings:
https://travis-ci.org/github/beetbox/beets/jobs/662725636#L912

Would you mind also adding a quick changelog entry describing the fix?

Thanks again for addressing this!

adamjakab · 2020-03-15T19:41:09Z

test/test_query.py

+    def test_query_repr(self):
+        fld = u'rg_track_gain'
+        if sys.version_info <= (3, 0):
+            self.assertEquals("NoneQuery(u'{}', True)".format(fld),
+                              str(NoneQuery(fld)))
+            self.assertEquals("NoneQuery(u'{}', True)".format(fld),
+                              str(NoneQuery(fld, fast=True)))
+            self.assertEquals("NoneQuery(u'{}', False)".format(fld),
+                              str(NoneQuery(fld, fast=False)))
+        else:
+            self.assertEquals("NoneQuery('{}', True)".format(fld),
+                              str(NoneQuery(fld)))
+            self.assertEquals("NoneQuery('{}', True)".format(fld),
+                              str(NoneQuery(fld, fast=True)))
+            self.assertEquals("NoneQuery('{}', False)".format(fld),
+                              str(NoneQuery(fld, fast=False)))
+


I am too n00b to Python to understand what is the story with unicode there. This is not really part of the issue but I though with this we have all methods of the class covered. Can you pls have a look?

TBH I think we can just remove the tests for repr… the problem is that Python 2 string literals used syntax like u'foo' while on Python 3 they are just 'foo'. Fortunately, repr formatting is so standard that IMO testing it is sort of overkill.

adamjakab · 2020-03-15T19:45:43Z

beets/dbcore/query.py

+    def value_match(self, pattern, value):
+        return value is pattern


I think this is ok. Also becuause the call from the match method also uses the self.patern initiated in the init.
Again, I am to new to Python to tell. Something like this in Php (i.e. not implementing an abstract method) would fail at compile time... but then again this is not php. So your call: should I remove it?

Let's just remove it. It should never be called, so I think it would just be misleading to have code here that looks like it should be meaningful (even though it's not). The earlier version you had, where match itself used get and compared to None, seems right to me.

sampsyo · 2020-03-16T01:56:28Z

beets/dbcore/query.py

+    def value_match(self, pattern, value):
+        return value is pattern


Let's just remove it. It should never be called, so I think it would just be misleading to have code here that looks like it should be meaningful (even though it's not). The earlier version you had, where match itself used get and compared to None, seems right to me.

beets/dbcore/query.py

docs/changelog.rst

sampsyo · 2020-03-16T02:04:03Z

test/test_query.py

+    def test_query_repr(self):
+        fld = u'rg_track_gain'
+        if sys.version_info <= (3, 0):
+            self.assertEquals("NoneQuery(u'{}', True)".format(fld),
+                              str(NoneQuery(fld)))
+            self.assertEquals("NoneQuery(u'{}', True)".format(fld),
+                              str(NoneQuery(fld, fast=True)))
+            self.assertEquals("NoneQuery(u'{}', False)".format(fld),
+                              str(NoneQuery(fld, fast=False)))
+        else:
+            self.assertEquals("NoneQuery('{}', True)".format(fld),
+                              str(NoneQuery(fld)))
+            self.assertEquals("NoneQuery('{}', True)".format(fld),
+                              str(NoneQuery(fld, fast=True)))
+            self.assertEquals("NoneQuery('{}', False)".format(fld),
+                              str(NoneQuery(fld, fast=False)))
+


TBH I think we can just remove the tests for repr… the problem is that Python 2 string literals used syntax like u'foo' while on Python 3 they are just 'foo'. Fortunately, repr formatting is so standard that IMO testing it is sort of overkill.

Co-Authored-By: Adrian Sampson <adrian@radbox.org>

adamjakab · 2020-03-16T12:18:02Z

@sampsyo - she is ready to go ;)

sampsyo · 2020-03-16T13:07:56Z

Looks excellent; thanks!! ✨

Adam Jakab added 4 commits March 15, 2020 12:52

match method converted to instance method

8988d90

added missing abstract method

eb4d2ef

removed unnecessary KeyError test

93744ff

removed value_match method - not reachable?

d6538e5

Adam Jakab added 4 commits March 15, 2020 15:23

value_match is now correctly implemented

238f224

better repr testing

3f2f125

better repr testing beetbox#2

532c6d7

cleaning up

8e68b5f

Adam Jakab added 3 commits March 15, 2020 19:19

fixing repr tests

935768d

struggling with old python

ceb901f

typo fix

ac1a385

adamjakab commented Mar 15, 2020

View reviewed changes

changelog entry

4c69939

sampsyo reviewed Mar 16, 2020

View reviewed changes

adamjakab and others added 2 commits March 16, 2020 10:12

sampsyo's docs/changelog.rst correction

4ba7b8d

Co-Authored-By: Adrian Sampson <adrian@radbox.org>

removed value_match and repr tests

611659d

sampsyo merged commit 5d39d02 into beetbox:master Mar 16, 2020

sampsyo added a commit that referenced this pull request Mar 16, 2020

Slightly terser get call for #3517

b34d1f7

adamjakab deleted the none_query_match branch March 26, 2020 08:48

snejus mentioned this pull request Dec 5, 2024

Release: Fix changelog formatting #5529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3516 - Erroneous @classmethod on NoneQuery.match#3517

Fix #3516 - Erroneous @classmethod on NoneQuery.match#3517
sampsyo merged 14 commits intobeetbox:masterfrom
adamjakab:none_query_match

adamjakab commented Mar 15, 2020 •

edited

Loading

Uh oh!

adamjakab commented Mar 15, 2020

Uh oh!

sampsyo commented Mar 15, 2020

Uh oh!

adamjakab Mar 15, 2020

Uh oh!

sampsyo Mar 16, 2020

Uh oh!

adamjakab Mar 15, 2020 •

edited

Loading

Uh oh!

sampsyo Mar 16, 2020

Uh oh!

sampsyo Mar 16, 2020

Uh oh!

Uh oh!

Uh oh!

sampsyo Mar 16, 2020

Uh oh!

adamjakab commented Mar 16, 2020

Uh oh!

sampsyo commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		def value_match(self, pattern, value):
		return value is pattern

Conversation

adamjakab commented Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

adamjakab commented Mar 15, 2020

Uh oh!

sampsyo commented Mar 15, 2020

Uh oh!

adamjakab Mar 15, 2020

Choose a reason for hiding this comment

Uh oh!

sampsyo Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

adamjakab Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sampsyo Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

sampsyo Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sampsyo Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

adamjakab commented Mar 16, 2020

Uh oh!

sampsyo commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamjakab commented Mar 15, 2020 •

edited

Loading

adamjakab Mar 15, 2020 •

edited

Loading