Closed lk-geimfari closed 6 years ago
As for me, I like the idea, but with some notes:
enums
we should import ones, if we want to change default values of arguments of the methods.@sobolevn So, i refactored check_gender
using enum
and here how it looks now:
from mimesis.enums import Gender
from mimesis.exceptions import UnexpectedGender
def check_gender(gender: Gender = Gender.RANDOM) -> str:
if gender in Gender:
return gender.value
else:
raise UnexpectedGender(
'You should use Gender from the module mimesis.enums')
If it's correct way to use enum
, then i really like how it looks. This is awesome!
P.S An old solution was awful.
Yes, it looks nice. But what is Gender.DEFAULT
? It is not clear to me without going into the docs. It really should be Gender.MALE
.
@sobolevn It's for returning random gender, maybe it's good idea to rename it to RANDOM
?
@sobolevn Should we use enums in methods everywhere or only where we have try/except/KeyError
?
@lk-geimfari sorry, I don't get it. Could you provide a code example to illustrate your point?
@sobolevn For example
def mime_type(self, type_t: str = 'application') -> str:
"""Get a random mime type from list.
:param str type_t:
Type of media: (application, image, video, audio, text, message).
:return: Mime type.
:raises ValueError: if type_t is not supported.
"""
supported = ' '.join(MIME_TYPES.keys())
if type_t not in list(MIME_TYPES.keys()):
raise ValueError(
'Unsupported mime type! Use: {}'.format(supported))
mime_type = self.random.choice(MIME_TYPES[type_t])
return mime_type
We can rewrite it using enums
like this:
def mime_type(self, type_: MimeType = MimeType.RANDOM) -> str:
"""Get a random mime type from list.
:param type_: Enum object MimeType.
:return: Mime type.
:raises ValueError: if type_t is not supported.
"""
if type_ and type_ in MimeType:
return self.random.choice(MIME_TYPES[type_.value])
else:
raise NonEnumerableError("MimeType")
I want to understand which solution is clearer in terms of design. Or, perhaps, both solutions are bad?
I like the second one. That was my original point, when I have fired an issue.
I am not sure about RANDOM
by the way. I have heard a lot of developers said that it does not belong there. That enum
represent items. RANDOM
is not an item.
Maybe it is not to late to replace it with a function?
def mime_type(self, type_: Optional[MimeType] = None) -> str:
if type_ is None:
type_ = select_random_item(MimeType)
...
return self.random.choice(MIME_TYPES[type_.value])
This proposal was originally borned in our discussion here.
Proposal (by @sobolevn):
"Maybe we could use enums more often? It really covers this case easily. And it is possible to statically check your code with enum."
Here's the sample code:
And then:
So, we want to discuss with community what is better? Let's discuss.
If you like this idea, add :+1:, otherwise :-1:.