Skip to content
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

Seed does not work for some providers #325

Closed
lk-geimfari opened this issue Dec 18, 2017 · 30 comments

Comments

Projects
None yet
2 participants
@lk-geimfari
Copy link
Owner

commented Dec 18, 2017

Description:
Seems like @duckyou have found a bug in our code and seed is really does not work sometime and we should fix it.

Reason:
It's happening because we use custom_code which use another random object.

Decision:
We should move utils which use random to helpers.Random

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

Thanks for the quick response, @lk-geimfari 😄

I can try moving custom_code to BaseProvider like you said in #324 discussion

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou You know, it's better to move it to helpers.Random. There's just everything related to random selection.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou after that, we can just call self.random.custom_code. Looks good.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou If you don't want to do it, I can do it by myself, but if you want work on it, then, please, create new PR, because old is really hard to read and review 😄 . Thanks!

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

@lk-geimfari Hmm... You goddamn right!
That's may be simplest solution ☺️

I do this ;)

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou Thank you!

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

@lk-geimfari

class AllowRandom(EnumMeta):
"""This metaclass allows method ``get_random_item()`` which
returns random field of enum.
"""
def get_random_item(self) -> Any:
return choice(list(self))

Method get_random_item also affected 😟

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou We will not change it, because it's not really about data, but about arguments for function.

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

@lk-geimfari Issue explain:

>>> from mimesis import Address
>>> from mimesis.enums import CountryCode
>>> a = Address(seed=42)
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'NIC'
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'BRN'
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'ARM'
>>> a = Address(seed=42)
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'NIC'
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'BRN'
>>> a.country_iso_code(fmt=CountryCode.ISO3)
'ARM'
...

This worked because we set format directly
... but if we omit this parameter

...
>>> a = Address(seed=42)
>>> a.country_iso_code()
'558'
>>> a.country_iso_code()
'096'
>>> a.country_iso_code()
'ARM'
>>> a = Address(seed=42)
>>> a.country_iso_code()
'NIC'
>>> a.country_iso_code()
'096'
>>> a.country_iso_code()
'051'

its happen beause:

if fmt is None:
fmt = CountryCode.get_random_item()

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 18, 2017

@duckyou Skip this for now. I'll think about it later. Our priority for a right now is custom_code.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 20, 2017

Fixed. Now we should think about how should we test providers with seed.

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@lk-geimfari good job! 😸
Sorry for off-top, but maybe this project community have a place for discussions.. for example irc or slack?

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 20, 2017

@duckyou It's okay. Unfortunately, not yet. We have a small community, so usually, we discuss everything here, although it would be great, of course, to have a place where we could discuss everything.

We can create repository discussion in @mimesis-lab. I can add you to the team if you're interested in this project.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 20, 2017

@duckyou I have invited you to @mimesis-lab. Accept the invitation if you're interested. Thanks!

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@lk-geimfari im in! ^^

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@lk-geimfari don't forget about #325 (comment) ?

Heres two problems: AllowRandom.get_random_item and ValidateEnumMixin._validate_enum

Ok.. firstly how to get random enum with seed?
Original realisation is:

mimesis/mimesis/enums.py

Lines 11 to 12 in 217e5fa

def get_random_item(self) -> Any:
return choice(list(self))

aka we can just do this self.random.choice(list(CountryCode)) without mentioned method

and if we create this simple hack in AllowRandom metaclass:

    def __getitem__(self, index):
        if isinstance(index, int):
            return list(self._member_map_)[index]
        return super().__getitem__(index)

we can do now self.random.choice(CountryCode) 😉

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou Also we can add the method get_random_item() to helpers.Random.

def get_item(self, enum: Optional[Any] = None):
    return self.choice(list(self)) 

What do you think? In my opinion, it will be easier.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

In this case, we should move _validate_enum() to BaseProvider

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@lk-geimfari
hmmm self.random.get_enum_item(CountryCode) or self.random.choice(CountryCode)
... also enum attribute always should be passed to get_enum_item method
but if we combine this with _validate_enum...

key = self.random.get_enum_item(fmt, CountryCode)

Something like this:

def get_enum_item(self, enum: Any, item: Any = None) -> Any:
    if item:
        raise NonEnumerableError(enum) if item not in enum
        return item
    return self.choice(list(enum))
@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou Stop stop. Seems like the solution which I suggested above will not work, it seems.

Do you suggest something like this?

class Enum(enum.Enum):
    def __getitem__(self, index):
        if isinstance(index, int):
            return list(self._member_map_)[index]
        return super().__getitem__(index)

class PortRange(Enum):
    ALL = (1, 65535)
    WELL_KNOWN = (1, 1023)
    EPHEMERAL = (49152, 65535)
    REGISTERED = (1024, 49151)
@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou The problem that we need to handle different cases. For example, sometimes we need the value of the enum, but sometimes we need item of enum object, i.e Enum.ITEM, not Enum.ITEM.value.

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

Oh... here is:

class MimEnumMeta(EnumMeta):
    def __getitem__(self, index):
        if isinstance(index, int):
            return list(self)[index]
        return super().__getitem__(index)


class PortRange(Enum, metaclass=MimEnumMeta):
    ALL = (1, 65535)
    WELL_KNOWN = (1, 1023)
    EPHEMERAL = (49152, 65535)
    REGISTERED = (1024, 49151)

...

Now:

>>> from mimesis import enums
>>> import random
>>> pr = enums.PortRange
>>> pr
<enum 'PortRange'>
>>> pr['ALL']
<PortRange.ALL: (1, 65535)>
>>> pr.ALL
<PortRange.ALL: (1, 65535)>
>>> pr[0]
<PortRange.ALL: (1, 65535)>
>>> random.choice(pr)
<PortRange.REGISTERED: (1024, 49151)>
>>> random.choice(pr) in enums.PortRange
True
>>> random.choice(pr).value
(1, 1023)
>>> random.choice(pr).name
'EPHEMERAL'
@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou I'll wait for your PR. Keep in mind that we have builtins data providers and that sometimes we need an item of the enum, but not a value (personla.py -> surname, payment.py -> credit_card_number).

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@lk-geimfari Can i also move _validate_enum() to BaseProvider, like you say here #325 (comment) ?

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou If we do it, then tests will fail because ValidateEnumMixin used in mimesis.builtins. It's really hard to understand what is better. At this moment we can only experiment with solutions.

@duckyou

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@lk-geimfari I see here several solutions:

  1. Just pass random object to method
  2. Like you say here #325 (comment)
  3. Init random object in ValidateEnumMixin
  4. Just in provider method validate enum and raise exception
@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou Let's will weigh the pros and cons of all solutions.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou We need to take a decision that will allow us to make the minimum number of changes.

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

@duckyou I have found a new problem:
Seeded full_name() returns different data because there are two different methods with similar seed, i.e name and surname.

And this is only the tip of the iceberg I think. Now I'm tired and can not figure out why the data is changing for fixed seed, sorry.

I have created PR where you can see changes: #332

@lk-geimfari

This comment has been minimized.

Copy link
Owner Author

commented Dec 21, 2017

Seems like builtins providers also does not support of seed. We should fix it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.