feat: support customizing attr getter in SubFactory

[ryancausey]

This change should allow for the case where a SelfAttribute attempts to access the field of a SubFactory that is a DictFactory. The user can override the deepgetattr_func of the SubFactory to use deepdictgetattr, or one of their own choosing, which will be used to retrieve the attribute.

closes #1134

[francoisfreitag]

Thanks for the issue and pull request!

I’m not sure about the behavior of silently switching to __getitem__ in case __getattr__ failed. Users may have subclassed __getitem__ on a class and not expect that code to run from SelfAttribute().

The documented behavior is:

Some fields should reference another field of the object being constructed, or an attribute thereof.

field is more general and would apply to a dict entry.

The docstring were clearly written with attribute fetching in mind.

Because DictFactory is not documented, I am inclined to stick with the current behavior to avoid surprises, especially since the suggested patch has no opt-out. Maybe an option forward would be to make the resolver function configurable (defaulting to the current deepgetattr), so that interested users can implement their own resolution?

[ryancausey]

Thanks for the issue and pull request!

I’m not sure about the behavior of silently switching to __getitem__ in case __getattr__ failed. Users may have subclassed __getitem__ on a class and not expect that code to run from SelfAttribute().

The documented behavior is:

Some fields should reference another field of the object being constructed, or an attribute thereof.

field is more general and would apply to a dict entry.

The docstring were clearly written with attribute fetching in mind.

Because DictFactory is not documented, I am inclined to stick with the current behavior to avoid surprises, especially since the suggested patch has no opt-out. Maybe an option forward would be to make the resolver function configurable (defaulting to the current deepgetattr), so that interested users can implement their own resolution?

What would this look like? Some parameter passed to SelfAttribute?

[francoisfreitag]

Maybe an option forward would be to make the resolver function configurable (defaulting to the current deepgetattr), so that interested users can implement their own resolution?

What would this look like? Some parameter passed to SelfAttribute?

Yes, that’s what I had in mind.

[ryancausey]

@francoisfreitag so what I am hearing based on this:

Maybe an option forward would be to make the resolver function configurable (defaulting to the current deepgetattr), so that interested users can implement their own resolution?

Is that there should be a new parameter added to SubFactory to define a custom callable to retrieve attributes and nested attributes. This callable should default to deepgetattr unless otherwise specified. Is this a correct understanding?

[francoisfreitag]

I meant to add a kwarg to SelfAttribute (something like (deepgetattr_func=deepgetattr), defaulting to the current deepgetattr). It would allow users to subclass SelfAttribute with a different attribute getter implementation, one that would follow dict __getitem__ in your case.

The main goal is to keep the current implementation, because the proposed call to __getitem__ is unexpected, but allow you to extend SelfAttribute to use the dictgetattr implementation in your project.

[ryancausey]

Got it. I will make the changes. I will also add a dictgetattr implementation to the library so it's readily available for others unless told not to.

[ryancausey]

@francoisfreitag I've made the suggested changes. Let me know if this is acceptable.

[rbarrois]

Hello,

I'm truly sorry, but I'm not going to merge this, since factory.LazyAttribute(lambda o: o.some_subfactory["my_field"]) does the job.
I've spent quite some time over the year removing magic and implicit behaviour from the library…

As the Zen of Python says: There should be one-- and preferably only one --obvious way to do it.

By the way: it's easier for us if discussion about possible implementations/solutions are made in an issue, before jumping to "let's add some code" — the less code, the easier it is for us to maintain.

[ryancausey]

Hello,

I'm truly sorry, but I'm not going to merge this, since factory.LazyAttribute(lambda o: o.some_subfactory["my_field"]) does the job. I've spent quite some time over the year removing magic and implicit behaviour from the library…

As the Zen of Python says: There should be one-- and preferably only one --obvious way to do it.

By the way: it's easier for us if discussion about possible implementations/solutions are made in an issue, before jumping to "let's add some code" — the less code, the easier it is for us to maintain.

@rbarrois thanks for the consideration. I didn't have time to wait to discuss so I made a fix that worked for me in the spirit of what I thought FactoryBoy was trying to achieve by offering a DictFactory. I tried to be a good citizen and contribute it back rather than leave it hidden in some fork. FWIW if there is not going to be a behavior change to SubFactory to allow what this MR tried to do, then you should probably remove factory.DictFactory altogether or change it to use the GetAttrDict suggested in #1134 (comment) so folks don't run into the surprising behavior of some factory boy declarations not working with DictFactory.