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

feat(fields): implement Param and CallableParam for handling unmapped parameters that can be referenced during build #650

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

leverage-analytics
Copy link

This completes the development and testing related to #604.

  • Implement new Param and CallableParam field types
  • Update handling of **kwargs for base factory process_kwargs and process_kwargs_coverage. A new helper method was used to determine which fields in a factory are Unmapped types (Params or CallableParam)
  • Implement tests for new fields as well as changes to factory build
  • Update documentation to include overview of feature functionality with examples

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/650

@@ -114,3 +115,121 @@ def to_value(self) -> Any:

msg = "fixture has not been registered using the register_factory decorator"
raise ParameterException(msg)


class NotPassed:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the existing Null type be used for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had originally thought the same thing but, in the end, decided to define a different constant just so that I wouldn't be using the existing Null in an unintended manner.

I'm happy to make this change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently used in multiple places to represent this so this fine to reuse

return cast(T, self.param)


class CallableParam(Generic[T], BaseParam[T, Callable[..., T]]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case this? Param type itself allows new functionality but this seems like adding bespoke syntax for a less common case

I think supporting default_factory or similar on Param itself cover this use case without having to deal with arg or kwargs and can unify this into one class

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my original use case, I needed the parameter to be a callable. Imagine an object like this:

import dataclasses
from zoneinfo import ZoneInfo
from polyfactory.factories import DataclassFactory

@dataclasses.dataclass
class Demo:
    name: str
    city: str
    time_zone: ZoneInfo

class DemoFactory(DataclassFactory[Demo]):
    @classmethod
    def city(cls) -> str:
        return cls.__faker__.city()
    
    @classmethod
    def time_zone(cls) -> ZoneInfo:
        return cls.__faker__.pytimezone()

It is trivial to create a simple factory and instances for this type. In this example, however, we will most likely have nonsensical objects (e.g. Demo(name='hello',city='London',time_zone=ZoneInfo('America/Chicago')). Of course, this behavior is expected, but what if we want to generate objects that more closely resemble the objects we're testing?

If we have a callable parameter type, we can then do something like this instead. Note: just for the sake of conversation, I'm going to be using the vocabulary I introduced.

from polyfactory.factories import DataclassFactory
from polyfactory.decorators import post_generated
from polyfactory.fields import CallableParam

class DemoFactory(DataclassFactory[Demo]):
    # the geo provider has a location_on_land that returns a tuple containing latitude, longitude, place name, two-letter country code, timezone
    location = CallableParam[tuple[str, ...]](lambda: DataclassFactory.__faker__.location_on_land())
    
    @postgenerated
    @classmethod
    def city(cls, location: tuple[str, ...]) -> str:
        return location[2]
    
    @postgenerated
    @classmethod
    def time_zone(cls, location: tuple[str, ...]) -> ZoneInfo:
        return location[4]

Now when an object is created, a single draw for location is made, and that tuple is used to populate the two attributes whose values are closely related.

That being said, I do really like your proposal for keeping it all in one class and using a default_factory param for the constructor. I can refactor and resubmit a pull request if you like.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you. The use case for a callable makes sense so can use existing functions/faker instance.

I think just allowing a callable that gets resolved should cover this and also makes sense if want to support decorator form like with postgenerated or similar. I think the interface of have a callable passed in build that gets called with params is less clear and is less like other existing fields. This can be done on user side so think is fine to not support

That being said, I do really like your proposal for keeping it all in one class and using a default_factory param for the constructor. I can refactor and resubmit a pull request if you like.

I'd keep it in the same PR and update in place. Happy to go with either

  1. A separate class that takes in a callable resolved at runtime with args/kwargs passed to function with build time behaviour being fully overridden. This matches behaviour of other fields
  2. Combine these and use default_factory or similar

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a convention being used for tests using __ between methods for these new? This convention is not similar to existing tests so not sure should introduce here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct; I can fix that.

@@ -1016,7 +1048,7 @@ def process_kwargs(cls, **kwargs: Any) -> dict[str, Any]:
for field_name, post_generator in generate_post.items():
result[field_name] = post_generator.to_value(field_name, result)

return result
return {key: value for key, value in result.items() if key not in params}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this logic work for when Param is used against a factory field? This would raise an error for kwargs required by the model in potential unclear way. Should an error be explicitly raised in this case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wanted to make sure I'm understanding what you mean by param is used against a factory field. Could you please provide an example?

Thanks for reviewing my pull request!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from dataclasses import dataclass

from polyfactory.factories import DataclassFactory
from polyfactory.fields import Param


@dataclass
class MyModel:
    values: int


class MyModelFactory(DataclassFactory[MyModel]):
    values = Param(1)


print(MyModelFactory.build())

take this example. This will fail as values will be omitted from final kwargs. Should Param be checked against factory fields? Maybe in __init_subclass__?

Thanks for reviewing my pull request!

And thank you for submitting! Great feature and work on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants