deltachat-core-rust icon indicating copy to clipboard operation
deltachat-core-rust copied to clipboard

remove Chat::get_info_json()?

Open r10s opened this issue 2 years ago • 4 comments

get_info_json() was added some years ago with the goal to help desktop getting information about a a chat.

i came over this function when working on #3520, i thought, that may be a nice function, just add a field, no new api, done.

however, looking at the code, drawbacks of get_info_json() are, that it does no just write the internal chat info to json - but also does quite some more expensive things that result in several db calls.

so, if we would have added the "mailing list" address here, reading it would result in quite some overhead, therefore i decided against it and added a separate ffi.

having a closer look, get_info_json() seems unmaintained and also incomplete, eg. pinning is missing. diving deeper, it looks as if the function is mostly unused, i could not find any reference to it even on desktop.

there is only one call to it from python that exposes it under the bit misleading name get_summary() - idk, if that is used.

if it turns out, the function is not used, i suggest to remove it:

  • desktop goes for a much broader json-rpc
  • in general, i like the idea of getting a structure returned like that, that may be also useful for ios/android, and json is alread in use meanwhile, but i would prefer not to mix returning already loaded structure fields and things that require database access, this is a waste of resources most times.

tl;dr: main question: is the function used in python in real world? if so, it may be easier to close this pr :)

r10s avatar Jul 26 '22 10:07 r10s

shouldn't we deprecate it first before outright removing it? That being said I think this experiment didn't ever make it into desktop's master/main branch.

Simon-Laux avatar Jul 26 '22 11:07 Simon-Laux

shouldn't we deprecate it first before outright removing it? That being said I think this experiment didn't ever make it into desktop's master/main branch.

yip, i think for the ffi users, we can just remove it.

the question is if it is used by python users.

r10s avatar Jul 26 '22 14:07 r10s

the question is if it is used by python users.

I remember reading it and found it incomplete so didn't used it for deltachat-cursed TUI client

I think it is okish to remove it, we don't have a lot of devs creating custom clients :D but would love if we start using semantic versions, at least for python package that way a dev can define their dependencies without worrying too much about breaking changes

adbenitez avatar Jul 26 '22 20:07 adbenitez

python tests fail as three lines use get_summary in assert; that needs to be substituted by the direct calls if we want to merge this pr.

r10s avatar Aug 15 '22 22:08 r10s

let's close this pr, the function seems to be used at least a little ...

r10s avatar Nov 17 '22 11:11 r10s