feat: Add copyWith method to all models #624
Conversation
github-actions
bot
added
🌱 Eco-Score
📖 Knowledge panels
product
pubspec
🤖 Robotoff
labels
There was a problem hiding this comment.
Hi @M123-dev!
A lot to review, but each time it's rather quick!
TBH, I cannot picture exactly the added value of that "CopyWith" feature, more specifically in the context of openfoodfacts/smooth-app#3059 and openfoodfacts/smooth-app#3263.
In those Smoothie issues, we need to "merge" two Products (the second on top on the first, only for the second's non null fields), like inputProduct.merge(product: changes), or Product.merge(product1, product2).
And from what I saw in the generated code it's just about individual fields. Like product1.copyWith(lang: 'de').
If so, that would mean that we would need to list all the fields of the second product and then use copyWith just on it, which is more a pain in the neck than a good feature.
And in dart, you can already write easily Product(barcode: '1234')..lang='de'..website='https://...'.
Therefore, I think we need at least a relevant example of the use of copyWith. Would you please share one?
Beyond that, you introduced slight breaking changes, but as it's for classes that are not very interesting I'm sure you'd agree to rollback your changes there, as you suggested in the OP.
| @JsonSerializable() | ||
| class TaxonomyAdditive extends JsonObject { | ||
| TaxonomyAdditive( | ||
| TaxonomyAdditive({ |
| @JsonSerializable() | ||
| class TaxonomyAllergen extends JsonObject { | ||
| TaxonomyAllergen( | ||
| TaxonomyAllergen({ |
| @JsonSerializable() | ||
| class TaxonomyCategory extends JsonObject { | ||
| TaxonomyCategory( | ||
| TaxonomyCategory({ |
| @JsonSerializable() | ||
| class TaxonomyCountry extends JsonObject { | ||
| TaxonomyCountry(); | ||
| TaxonomyCountry({ |
There was a problem hiding this comment.
Did copy_with force you to do that?
| @JsonSerializable() | ||
| class TaxonomyIngredient extends JsonObject { | ||
| TaxonomyIngredient( | ||
| TaxonomyIngredient({ |
| @JsonSerializable() | ||
| class TaxonomyOrigin extends JsonObject { | ||
| TaxonomyOrigin(); | ||
| TaxonomyOrigin({ |
There was a problem hiding this comment.
Did copy_with force you to do that?
| @JsonSerializable() | ||
| class TaxonomyPackaging extends JsonObject { | ||
| TaxonomyPackaging(); | ||
| TaxonomyPackaging({ |
| @JsonSerializable() | ||
| class TaxonomyPackagingMaterial extends JsonObject { | ||
| TaxonomyPackagingMaterial(); | ||
| TaxonomyPackagingMaterial({ |
| @JsonSerializable() | ||
| class TaxonomyPackagingRecycling extends JsonObject { | ||
| TaxonomyPackagingRecycling(); | ||
| TaxonomyPackagingRecycling({ |
| @JsonSerializable() | ||
| class TaxonomyPackagingShape extends JsonObject { | ||
| TaxonomyPackagingShape(); | ||
| TaxonomyPackagingShape({ |
|
Ohh, my bad @monsieurtanuki it makes sense that these methods do not make sense (at least for us), I was a bit hasty with the implementation. |
|
Thank you anyway for your attempt @M123-dev! |
|
So looking at the problem how we actually have it.
|
|
@M123-dev Actually I think I had a specific need like that in openfoodfacts/smooth-app#3339 (I created a Should we generalize that? I don't know. For the moment we only have this specific need for Another grander idea would be to code a bit like in If you feel like refactoring in general, I would suggest #450. Would impact all files, with breaking changes. But the code would be cleaner afterwards. |
What
Adds copyWith methods to all JsonSerializable models we have
My first try was to use freezed as this can add a copyWith method to JsonSerializable but because we have a lot of customization around languages and so on I decided to go for this simpler method.
For most classes it was just adding a
@CopyWith()decorator others needed a change in the constructor to use named parameter.I added it to all classes in a first run, if there are some were we don't need it I'am happy to remove them
For
openfoodfacts/smooth-app#3059
openfoodfacts/smooth-app#3263
...