Keys remember whether to use deprecated syntax #6

sarina · 2014-05-30T19:17:53Z

and the deprecated key handler registers itself with the base key type
LMS-2752

sarina · 2014-05-30T20:22:08Z

Heads up Power Team LMS @cpennington @flowerhack @dianakhuang -- this is the start to https://edx-wiki.atlassian.net/browse/LMS-2752

cpennington · 2014-06-02T16:03:06Z

opaque_keys/__init__.py

@@ -172,7 +175,7 @@ def replace(self, **kwargs):
        return type(self)(**existing_values)

    def __setattr__(self, name, value):
-        if getattr(self, '_initialized', False):
+        if name != 'deprecated' and getattr(self, '_initialized', False):


Why are we allowing deprecated to flip after initialization?

@dmitchell ?

cpennington · 2014-06-02T16:09:06Z

Globally, deprecated should be passed in the constructor, so that keys continue to act like immutable values, and self.deprecated should be used preferentially to getattr and setattr.

sarina · 2014-06-02T18:24:53Z

Pulling in @dmitchell since these changes were originally authored by him. Don, can you look over this and help me respond to comments made?

sarina · 2014-06-02T19:59:19Z

wrt CourseKey.deprecated_fallback = SlashSeparatedCourseKey in opaque_keys/edx/locations.py

I worry a little about this silently breaking depending import order (if some other class has already set CourseKey.deprecated_fallback, this line will overwrite it). Perhaps a class method is in order on the base class to handle that?

@cpennington I'm not sure I'm following what you intend here - what do you want to handle, and how?

sarina · 2014-06-02T23:44:26Z

Heads up that I force-pushed the branch.

These changes are now done:

Globally, deprecated should be passed in the constructor, so that keys continue to act like immutable values.
Base class enforces that only one deprecated_fallback can be set.

sarina · 2014-06-03T03:50:15Z

opaque_keys/tests/test_opaque_keys.py

+    def test_hash_equality(self):
+        self.assertEquals(hash(DummyKey.from_string('hex:0x10')), hash(DummyKey.from_string('hex:0x10')))
+        # TODO: These two hash to the same value - is this a feature or a bug?
+        #self.assertNotEquals(hash(DummyKey.from_string('hex:0x10')), hash(DummyKey.from_string('base10:16')))


Was trying to increase test coverage and found that hashing two different keys w/ the same values leads to the same hash. Wanted to know if we consider this a feature (in which case will change this to an assertEquals) or a bug (in which case I'll change how __hash__ is implemented).

Two key objects of the same type with the same values should hash to the same value, so that they can be used in dictionaries as keys in a useful way. Things that compare equal should also hash equal.

Two keys with different implementation types shouldn't hash to the same value, though.

@cpennington thanks, that's what I thought. Right now,

hash(DummyKey.from_string('hex:0x10')) == hash(DummyKey.from_string('base10:16'))

while

DummyKey.from_string('hex:0x10') != DummyKey.from_string('base10:16')

I think the best way to fix the hash method may be to incorporate the CANONICAL_NAMESPACE into the __hash__ method, as opposed to just using hash(self._key). Thoughts?

Another option would be to include type(self) in self._key, and remove the check from __eq__.

sarina · 2014-06-03T17:24:57Z

@cpennington @dmitchell @flowerhack @dianakhuang I think this is ready for review

dmitchell · 2014-06-03T21:17:04Z

opaque_keys/__init__.py

@@ -184,6 +189,9 @@ def __unicode__(self):
        """
        Serialize this :class:`OpaqueKey`, in the form ``<CANONICAL_NAMESPACE>:<value of _to_string>``.
        """
+        if self.deprecated:
+            # no namespace on deprecated
+            return self._to_string()


I'm surprised we're using _to_string rather than to_deprecated_string. By using _to_string we're requiring the non-deprecated syntax to == the deprecated syntax sans the namespace giving us no flexibility to change serialization syntax. (or does _to_string also look for the deprecated flag?)

Ok, I see _to_string() does consult the deprecated flag; so, this is probably ok, but it's not quite parallel to from_string().

dmitchell · 2014-06-03T21:34:01Z

Who's removing locators.py, locations.py, & opaque_keys from edx-platform and moving test-locators, test_location to this module?

cpennington · 2014-06-10T16:36:40Z

already done in https://github.com/edx/edx-platform/pull/3862 and https://github.com/edx/edx-platform/pull/3866

and the deprecated key handler registers itself with the base key type LMS-2752

flowerhack · 2014-06-16T18:55:14Z

opaque_keys/__init__.py

@@ -130,7 +164,7 @@ def _separate_namespace(cls, serialized):
            MissingNamespace: Raised when no namespace can be
                extracted from `serialized`.
        """
-        namespace, _, rest = serialized.partition(cls.NAMESPACE_SEPARATOR)
+        namespace, __, rest = serialized.partition(cls.NAMESPACE_SEPARATOR)

        # If ':' isn't found in the string, then the source string
        # is returned as the first result (i.e. namespace)


Could we add a note here that we'd expect this to happen either in the case of (1) a totally borked string, or (2) a deprecated string? Otherwise it's a little confusing, reading through, that deprecated strings would be considered invalid

dianakhuang · 2014-06-16T20:32:35Z

I think that's my only other comment. Everything else is 👍

coveralls · 2014-06-16T21:35:00Z

Coverage increased (+8.17%) when pulling e028b5e on sarina/handle-old-new-keys-transparently into cf29dfe on master.

flowerhack · 2014-06-16T21:40:00Z

It wasn't introduced in this PR, but does anyone know the deal with the is_fully_specified function in locator.py? It's a fine function to have, but we don't seem to set a useful value for is_fully_specified anywhere, thus rendering it sort of pointless

flowerhack · 2014-06-16T21:43:36Z

besides that, ⛵

coveralls · 2014-06-17T14:55:42Z

Coverage increased (+8.17%) when pulling baf0eb5 on sarina/handle-old-new-keys-transparently into cf29dfe on master.

cpennington · 2014-06-17T15:32:28Z

opaque_keys/__init__.py

@@ -232,7 +285,7 @@ def __lt__(self, other):
        return self._key < other._key  # pylint: disable=protected-access

    def __hash__(self):
-        return hash(self._key)
+        return hash(self._key) + hash(type(self))


I think that rather than using + to add the hashes here, we should include type(self) in _key.

Probably at the end of the tuple, so that it's just a tiebreaker for comparisons.

cpennington · 2014-06-17T15:35:49Z

👍 after the hash fix.

coveralls · 2014-06-17T15:41:36Z

Coverage increased (+8.36%) when pulling 7800777 on sarina/handle-old-new-keys-transparently into cf29dfe on master.

…ed_string parallel constructions

…mented

…ons in SSCK

coveralls · 2014-06-17T16:04:16Z

Coverage increased (+8.36%) when pulling 69b154c on sarina/handle-old-new-keys-transparently into cf29dfe on master.

Keys remember whether to use deprecated syntax

cpennington reviewed Jun 2, 2014
View reviewed changes

sarina reviewed Jun 3, 2014
View reviewed changes

dmitchell reviewed Jun 3, 2014
View reviewed changes

Keys remember whether to use deprecated syntax

497eb54

and the deprecated key handler registers itself with the base key type LMS-2752

flowerhack reviewed Jun 16, 2014
View reviewed changes

cpennington reviewed Jun 17, 2014
View reviewed changes

Julia Hansbrough and others added 12 commits June 17, 2014 11:45

Add more tests for opaque-keys

76d94a1

Reorganize OpaqueKey methods by function

9cd897c

Make _to_string/_from_string and _to_deprecated_string/_from_deprecat…

7c9330e

…ed_string parallel constructions

Don't measure coverage for abstract base methods that return NotImple…

fad77bb

…mented

Use _to_deprecated_string for serializing deprecated strings

a4a0194

Respond to review comments

c611e75

Clean up awkward deprecated argument passing

a33bd04

Add tests of Location deprecated son code

7c0f81d

Separated out the deprecated and non-deprecated string parsing functi…

0beb073

…ons in SSCK

Fail the build if any coverage commands fail

68cfa15

Increase test coverage

f7fc7eb

Make tests more concise

69b154c

sarina added a commit that referenced this pull request Jun 17, 2014

Merge pull request #6 from edx/sarina/handle-old-new-keys-transparently

fc67e5e

Keys remember whether to use deprecated syntax

sarina merged commit fc67e5e into master Jun 17, 2014

sarina deleted the sarina/handle-old-new-keys-transparently branch June 17, 2014 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keys remember whether to use deprecated syntax #6

Keys remember whether to use deprecated syntax #6

sarina commented May 30, 2014

sarina commented May 30, 2014

cpennington Jun 2, 2014

sarina Jun 2, 2014

cpennington commented Jun 2, 2014

sarina commented Jun 2, 2014

sarina commented Jun 2, 2014

sarina commented Jun 2, 2014

sarina Jun 3, 2014

cpennington Jun 3, 2014

sarina Jun 3, 2014

cpennington Jun 3, 2014

sarina commented Jun 3, 2014

dmitchell Jun 3, 2014

dmitchell Jun 3, 2014

dmitchell commented Jun 3, 2014

cpennington commented Jun 10, 2014

flowerhack Jun 16, 2014

dianakhuang commented Jun 16, 2014

coveralls commented Jun 16, 2014

flowerhack commented Jun 16, 2014

flowerhack commented Jun 16, 2014

coveralls commented Jun 17, 2014

cpennington Jun 17, 2014

cpennington Jun 17, 2014

cpennington commented Jun 17, 2014

coveralls commented Jun 17, 2014

coveralls commented Jun 17, 2014

Keys remember whether to use deprecated syntax #6

Keys remember whether to use deprecated syntax #6

Conversation

sarina commented May 30, 2014

sarina commented May 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpennington commented Jun 2, 2014

sarina commented Jun 2, 2014

sarina commented Jun 2, 2014

sarina commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarina commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitchell commented Jun 3, 2014

cpennington commented Jun 10, 2014

Choose a reason for hiding this comment

dianakhuang commented Jun 16, 2014

coveralls commented Jun 16, 2014

flowerhack commented Jun 16, 2014

flowerhack commented Jun 16, 2014

coveralls commented Jun 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpennington commented Jun 17, 2014

coveralls commented Jun 17, 2014

coveralls commented Jun 17, 2014