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

use Enum for rv type #40

Closed
Autoplectic opened this Issue Oct 22, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@Autoplectic
Copy link
Member

Autoplectic commented Oct 22, 2013

use Enum on py3.4+ and the backported enum34 package on all others.

at the moment this would be:

class RV_mode(Enum):
names = 0
indices = 1

and each dist would grow a type which is one of these enums. Information measures would look at this property to know how to do things.

@chebee7i

This comment has been minimized.

Copy link
Member

chebee7i commented Oct 26, 2013

So I'm definitely +1 on distributions storing a default value for this mode that is changed when rv names are assigned. The thing I'm +0 on is whether to use Enum or not. Its certainly conceptually clearer, but I find option A below easier for users to type and understand.

some_func(d, rv_mode='names')  # A
some_func(d, rv_mode=dit.RV_mode.names) # B

Either option is a big improvement over the current boolean rv_names=True. So I'm curious to hear what others think. It seems that we get the >90% of the functional benefit of enums just by checking if the parameter is in set(['indicies', 'names']) instead. [If rv_mode is None, then we grab the default stored on the distribution.]

@fiatflux

This comment has been minimized.

Copy link

fiatflux commented Oct 28, 2013

In most languages I'd be 100% for enum over a string label, so that problems can be caught early. But in Python typos will cause hairy runtime issues either way. So the only other consideration I can think of is high performance, which the very language is not.

tl;dr: I like string labels.

@chebee7i

This comment has been minimized.

Copy link
Member

chebee7i commented Feb 23, 2015

Addressed in #71.

@chebee7i chebee7i closed this Feb 23, 2015

@chebee7i

This comment has been minimized.

Copy link
Member

chebee7i commented Feb 23, 2015

And also in bffa2c6 from a while back.

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