Allow relation _id fields to use a sequence #253
Conversation
| elif field.name not in self.model_attrs: | ||
| if ( | ||
| not isinstance(field, ForeignKey) | ||
| or "{0}_id".format(field.name) not in self.model_attrs | ||
| or hasattr(field, "attname") |
There was a problem hiding this comment.
I swapped this to checking "attname" as it mirrors the same technique that Django uses internally.
| and hasattr(field, "attname") | ||
| and field.attname in self.iterator_attrs | ||
| ): | ||
| return False |
There was a problem hiding this comment.
I'm not a fan of returning a False here rather than running through the conditions and falling back to the False at the end. The last 2 conditions catch this scenario and it was getting complicated to isolate the right case. Suggestions welcome.
There was a problem hiding this comment.
I totally agree with you here @ashiazed, but I think this is a result of a major problem that is the fact that there's practically no abstractions under this major for field in fields loop.
I mean, this is done like that since the very beginning of the project and never was refactored. But today, it's more clear to me that we should have a piece of code to receive a field and generate a value/skip it. Baker class would only trust that everything happened as expected. If we had such object, I feel that these validations wouldn't be all grouped together like that.
tests/test_recipes.py
Outdated
| baker.make(Profile, _quantity=3) | ||
| seq_user = user_recipe.extend(username="name", profile_id=seq(1)) | ||
| user = seq_user.make() | ||
| assert user.profile_id == 2 |
There was a problem hiding this comment.
Just want to triple check here as I thought it was a tad unintuitive, the first generated number is 2 not 1?
Just want to make sure this didn't run through an unnecessary yield somewhere.
There was a problem hiding this comment.
That's correct. As you can see here, the iteration starts at start or increment_by (if start is falsey). Therefore, the only way to begin iteration from 0 is to supply a value of -1 and to begin from 1 to provide a value of 0 (which is definitely not intuitive).
I am not the original author, so I cannot speak to the intent of this code, however, it seems like a case of just trying to accommodate too many uses without a clear interface for each specific one. The code could probably be improved, however, most uses I have seen come in the form of str values with appended integers (which will "rightfully" start at Foo 1 by default). That is most likely the reason the int/float case hasn't gotten too much attention, but that is a guess on my part.
|
Happy Hacktoberfest 🎃 |
|
@timjklein36 Tests should be passing now. I was being silly and passed in a hardcoded int as a PK in the tests and of course that doesn't play nice in postgres test db 🙄 |
There was a problem hiding this comment.
Thanks @ashiazed for this PR and the discussions you've raised too. Given our merge policies, I have to wait for other maintainer to approve this PR before I can merge it.
| and hasattr(field, "attname") | ||
| and field.attname in self.iterator_attrs | ||
| ): | ||
| return False |
There was a problem hiding this comment.
I totally agree with you here @ashiazed, but I think this is a result of a major problem that is the fact that there's practically no abstractions under this major for field in fields loop.
I mean, this is done like that since the very beginning of the project and never was refactored. But today, it's more clear to me that we should have a piece of code to receive a field and generate a value/skip it. Baker class would only trust that everything happened as expected. If we had such object, I feel that these validations wouldn't be all grouped together like that.
Fixes #123
This change allows
seqto be used on anForeignKeyorOneToOne.I assume that in these cases the relations were made before hand and this type of recipe is used for assignment.