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

BUG: fix issues with sentinel #872

Merged
merged 5 commits into from Nov 24, 2015

Conversation

Projects
None yet
2 participants
@llllllllll
Member

llllllllll commented Nov 24, 2015

Also adds tests.

@llllllllll llllllllll added the Bug label Nov 24, 2015

def test_doc(self):
self.assertEqual(sentinel('a', 'b').__doc__, 'b')
def test_doc_differentiates(self):

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

I'm not sure this is useful behavior. I'd argue that this should just be an error.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

talked about this irl

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

(apparently slack is real life now)

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

I don't even know what is real anymore

self.assertIsNot(sentinel('a', 'b'), sentinel('a', 'c'))
def test_memo(self):
self.assertIs(sentinel('a'), sentinel('a'))

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Is there a benefit to giving this memoization semantics vs making this just fail if you construct two sentinels with identical names? I can't think of a scenario where I'd legitimately wantt two different "NotSpecified" sentinels lying around.

return value
raise ValueError(
'attempted to create sentinel with a used name and a different'

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

I'd capitalize the A here.

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

I think this template is expecting the two docstrings, but you're templating in the name and the existing docstring. How about this?

"""
New sentinel value %r conflicts with an existing sentinel of the same name.
Old Sentinel Docstring: %r
New Sentinel Docstring: %r
Resolve this conflict by changing the name of one of the sentinels.
"""

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

I like this.

)
@object.__new__ # bind a single instance to the name 'Sentinel'
class Sentinel(object):

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Is there a reason for preferring this construction over a call to type? It seems odd to create a class named Sentinel and then reset its name via __name__.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

the __name__ is only on the instance. This means that type(sentinel('a')).__name__ == 'Sentinel' so they are a little different. Also, this is giving us an instance directly without invoking the __new__. I just kind of prefer the class syntax but I don't have a huge attachment.

pass
return result
cls.__module__ = sys._getframe(1).f_globals['__name__']
except (AttributeError, ValueError, KeyError):

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

When do these exception cases get hit?

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

AttributeError isn't needed so I will cut it. ValueError is when the stack depth is not deep enough to return the f_back (basically this would need to be the top frame). The keyerror is if __name__ is not in the globals.

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

(basically this would need to be the top frame)

This code is running inside a function though; how could there not be a stack frame above us? Do we hit that case if this gets called directly from Python embedded in a C application?

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

Yeah, if someone called this function from a c library that was using libpython but didn't already have a python stack setup.

@@ -46,6 +46,8 @@ Bug Fixes
* Fixes an error raised in calculating beta when benchmark data were sparse.
Instead `numpy.nan` is returned (:issue:`859`).
* Fixed an issue pickling :func:`~zipline.utils.sentinel.sentinel` objects

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Period here.

raise ValueError(dedent(
"""\
New sentinel value %r conflicts with an existing sentinel of the

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Should there be a backslash here? I feel like wrapping the exception message isn't valuable here.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

There is no reason for the first character to be a newline.

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Agreed. I was suggesting that we add a backslash after the on the first line to make the first sentence render as one line in the error.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

ah, good call

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

You might want to check to make sure that that doesn't add a bunch of funky whitespace though.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

grrr, it does. I will just wrap it.

def __new__(cls):
raise TypeError("Can't construct new instances of %s" % name)
raise TypeError("Can't construct new instances of %r" % name)

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Period.

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

Hrm, looked at some stdlib examples and they don't put periods at the end. I think I disagree with that stylistically, but probably better to be consistent.

This comment has been minimized.

@llllllllll

llllllllll Nov 24, 2015

Member

I actually prefer omitting capitals and periods from error messages if it is a single sentence because it is less noisy. I could change the Can't to cannot to match: TypeError: cannot create 'NoneType' instances

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

I think I'd counter that we should make our error messages complete sentences. Grammar and punctuation exist because they help convey meaning. The fact that we're writing code doesn't make that suddenly not true.

This comment has been minimized.

@ssanderson

ssanderson Nov 24, 2015

Member

I'm bikeshedding real hard at this point though, so I'm not going to continue to harp on this.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Nov 24, 2015

Two missing periods and a backslash need adding and then this is good to merge.

@llllllllll llllllllll force-pushed the sentinel-fixes branch from b72462c to 7896520 Nov 24, 2015

@llllllllll llllllllll force-pushed the sentinel-fixes branch from 294ea7c to 0e246a2 Nov 24, 2015

@llllllllll

This comment has been minimized.

Member

llllllllll commented Nov 24, 2015

merging on pass

@llllllllll llllllllll merged commit 0e246a2 into master Nov 24, 2015

3 checks passed

Scrutinizer Analysis 19 updated code elements
Details
Scrutinizer Tests Tests are not configured
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@llllllllll llllllllll deleted the sentinel-fixes branch Nov 24, 2015

@ssanderson ssanderson added the Testing label Nov 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment