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

Deprecate Fuzzy Attributes that are already supported in Faker #271

Open
jeffwidman opened this Issue Feb 17, 2016 · 15 comments

Comments

Projects
None yet
9 participants
@jeffwidman
Member

jeffwidman commented Feb 17, 2016

I'd like to start a conversation around deprecating and eventually removing the Fuzzy Attributes that are already supported by Faker: http://factoryboy.readthedocs.org/en/latest/fuzzy.html

Reasoning:
I'd prefer to limit the scope of FactoryBoy to handling turning input data into models, including complicated scenarios like chaining. And then delegate the fuzzing of the data as much as possible to external libraries like Faker.

Since we now wrap Faker, there's no reason for us to provide FuzzyAttributes that are already provided by Faker--it's just duplicated work. For any duplicate FuzzyAttributes that are more powerful than their Faker equivalents, where possible, I'd rather see that code moved over to Faker.

@rbarrois your thoughts?

@rbarrois

This comment has been minimized.

Member

rbarrois commented Feb 21, 2016

Excellent idea! This made sense as long as faker wasn't supported or optional; but it's now providing a much better fuzzer for many functions.

We'll have to be careful on the upgrade path though — a backwards compatibility policy will soon be needed!

@jeffwidman

This comment has been minimized.

Member

jeffwidman commented Feb 21, 2016

Cool. Assuming this project is semvar, I think the todo's here are:

  • add deprecation note in docs immediately.
  • go through the fuzzy implementations and make sure the faker equivalent is at least as powerful; if not, submit that code as a PR to faker
  • actually remove the code as part of a 3.x.x release since it's a backwards incompatible change.

Could certainly handle it differently, but that's the route I'd go down for fast and easy. I'll try to start working on this, as I suspect getting things into a published Faker release might take a little while.

jeffwidman added a commit that referenced this issue Feb 21, 2016

Add note about deprecate/remove Fuzzy attributes
Full discussion in #271

Wanted to get something mentioned in the docs immediately.

@jeffwidman jeffwidman added this to the 3.0 milestone Jun 24, 2016

@abendebury

This comment has been minimized.

abendebury commented Aug 8, 2016

It seems that faker lacks a provider to pick a random choice out a of a list, like fuzzy.FuzzyChoice does.

@MichaelAquilina

This comment has been minimized.

MichaelAquilina commented Aug 12, 2016

I have also noticed that faker doesnt have an equivalent for FuzzyChoice. What's the recommended route. Should you use FuzzyChoice in that case or resort to creating your own in order to avoid using any fuzzy attributes?

@arugifa

This comment has been minimized.

arugifa commented Sep 13, 2016

Since fuzzy attributes (will) rely on an external dependency, what do you think about making their support optional? By putting something like extras_require = {'Fuzzy': ['fake-factory']} in setup.py.

@rbarrois

This comment has been minimized.

Member

rbarrois commented Sep 15, 2016

@alexandre-figura factory_boy already has a non-optional dependency on faker ;)

@rhunwicks

This comment has been minimized.

rhunwicks commented Oct 5, 2016

@PlasmaSheep said:

It seems that faker lacks a provider to pick a random choice out a of a list, like fuzzy.FuzzyChoice does

That's true. In the meantime, I have this:

TYPES = [e[0] for e in my_model.TYPE_CHOICES]

class MyFactory(factory.django.DjangoModelFactory):

    type = factory.Faker('random_element', elements=TYPES)

It doesn't work if you put the list comprehension directly into the factory.Faker call

@chrislawlor

This comment has been minimized.

chrislawlor commented Oct 24, 2016

Regarding the missing FuzzyChoice equivalent in Faker, you can avoid having to create new lists of valid choices by adding a new provider to the bundled Faker:

# somefile.py
class ChoiceProvider(BaseProvider):
    def random_choice(self, choices=None):
        """
        Given a list of choices in Django format, return a random value
        """
        choice = self.random_element(choices)
        return choice[0]

You then have to add the new provider:

from somefile import ChoiceProvider
factory.Faker.add_provider(ChoiceProvider)

Once the new provider has been added, use it like this:

class MyFactory(factory.django.DjangoModelFactory):
    choice_field = factory.Faker('random_choice', choices=mymodel.CHOICE_FIELD_CHOICES)

Having to include the add_provider boilerplate in all your factory files is a bit sub-optimal.
Personally, I feel it is a simple enough class that it could be added directly to factory boy instead of submitted as a PR to faker. I'm just not sure that it provides any value to faker outside the context of a django project using factory boy, so they'd be hesitant to accept it. I'd be happy to submit a PR here if anyone would like.

@abendebury

This comment has been minimized.

abendebury commented Oct 25, 2016

I'm just not sure that it provides any value to faker outside the context of a django project using factory boy,

It's applicable for a lot of fields - literally any field that requires a choice out of some concrete set of choices, so I don't see why it's limited to django.

@rbarrois

This comment has been minimized.

Member

rbarrois commented Oct 28, 2016

We could hide this with a minimal faker wrapper, e.g factory.FakerChoice(values)?
Behind the scenes, this could register an arbitrary provider with faker and use it.

The other option is to improve Faker to make it super-easy to declare a provider and use it on the fly.

@jeffwidman

This comment has been minimized.

Member

jeffwidman commented Oct 28, 2016

Wherever possible, I'd rather we improve Faker... Historically, I've found @fcurella to be very receptive to pull requests, etc.

Unless we absolutely have no alternative, I'm not excited about creating wrappers modifying behavior from an external library. Every time we do that, we introduce global complexity for anyone trying to use Faker with FactoryBoy because they now have another thing they need to understand at least well enough to evaluate whether or not it applies to their use case.

@fcurella

This comment has been minimized.

Contributor

fcurella commented Oct 28, 2016

Count me in for any improvement on Faker :)

Keep in mind that originally Faker was a port of a PHP library.

As a result, some provider methods may look a little weird, non-pythonic, and possibly slower for no reason. I've been meaning to go through them and fix them, just haven't had a good chance to do it, yet.

@kwasielew

This comment has been minimized.

kwasielew commented Nov 29, 2018

Hey all, do you have any rough idea when this deprecation will happen?
Thanks

@rbarrois

This comment has been minimized.

Member

rbarrois commented Nov 29, 2018

@kwasielew No strict roadmap has been planned yet; one of the points to keep in mind is that Faker provides little tools to restrict the possible values of fields.
This means that it would be complicated to replace, for instance, the FuzzyInteger or FuzzyDateTime fields without losing some ability to configure the range of values they should use.

@fcurella

This comment has been minimized.

Contributor

fcurella commented Nov 29, 2018

@kwasielew @rbarrois Feel free to submit issues and PRs that could help you on the Faker repo.

If you do, please include a link to this issue, so I can prioritize accordingly.

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