gino icon indicating copy to clipboard operation
gino copied to clipboard

Add extra_returning_fields in apply

Open kigawas opened this issue 4 years ago • 4 comments

Closes #730

kigawas avatar Mar 17 '21 07:03 kigawas

@kigawas, hi! Good PR & additional feature.

@wwwjfy, @fantix, what do you think?

xnuinside avatar May 03 '21 10:05 xnuinside

Sorry for the late.

I'm thinking of a parameter to return/reload every column instead of specified ones only. It saves a get call, which is probably being used in such case now.

Another nitpicky point is the usage of tuple in this PR. As I understand, list is more suitable here, semantically. Quoting Python doc: Tuples are immutable sequences, typically used to store collections of heterogeneous data

wwwjfy avatar Jun 20 '21 13:06 wwwjfy

Yes. Tuple should be replaced by Sequence in Python, which sugguests an immutable sequence although you can still pass list.

I'm thinking of a parameter to return/reload every column instead of specified ones only. It saves a get call, which is probably being used in such case now.

It's certainly better to get all columns by default, but for now a transitional approach would be more realistic to avoid API change

kigawas avatar Jun 20 '21 14:06 kigawas

I don't mean that big a change, but just an additional parameter like update_all (need a good name) to return all columns and update the instance. Similar to the one in the PR, but no need to list every single one.

wwwjfy avatar Jun 20 '21 15:06 wwwjfy