Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor DB view descriptions #182

Merged
merged 11 commits into from
Oct 21, 2019
Merged

Conversation

keckelt
Copy link
Contributor

@keckelt keckelt commented Aug 6, 2019

closes #181

@keckelt keckelt added the type: feature New feature or request label Aug 6, 2019
@keckelt keckelt requested a review from lehnerchristian August 6, 2019 10:50
@keckelt keckelt self-assigned this Aug 6, 2019
@thinkh thinkh changed the title Keckelt/id type view desc Add idType to DB view description Aug 7, 2019
@thinkh
Copy link
Member

thinkh commented Aug 8, 2019

We agreed in a personal meeting for the following changes:

  • Rename IViewDesc to IDatabaseViewDesc
  • Unify return values for API routes /tdp/db/publicdb/ and /tdp/db/publicdb/myview/desc so that they match the IDatabaseViewDesc
  • IViewDescription is only used by the PhoveaDataAdapter and should be moved there
  • ARankingView and AEmbeddedRankingView should use the IDatabaseViewDesc instead

@keckelt Please change the code accordingly and notify @lehnerchristian. Afterwards he will test the changes in different apps.

@thinkh thinkh changed the title Add idType to DB view description Refactor DB view descriptions Aug 8, 2019
@keckelt
Copy link
Contributor Author

keckelt commented Sep 2, 2019

@lehnerchristian @thinkh I changed the points we discussed, except:

  • IViewDescription is only used by the PhoveaDataAdapter and should be moved there
  • ARankingView and AEmbeddedRankingView should use the IDatabaseViewDesc instead

The RankingViews only use the interface to get the column description,so using the IDatabaseViewDesc is an overkill.
Actually it might be better to just return IServerColumn[] there, but I left it untouched for now as that might be best handled in another PR imo.

Copy link

@lehnerchristian lehnerchristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only two minor things. the rest looks good

src/rest.ts Outdated Show resolved Hide resolved
src/rest.ts Outdated Show resolved Hide resolved
@lehnerchristian
Copy link

TODO after merging: also merge dependent PRs where typings change

@keckelt
Copy link
Contributor Author

keckelt commented Sep 18, 2019

Ah, jetzt wo ich die member von IViewDescription optional gemacht habe ist natürlich IDatabaseViewDesc keine richtige Erweiterung:

Type 'Promise<Readonly<IDatabaseViewDesc>>' is not assignable to type 'Promise<IViewDescription>'.
  Type 'Readonly<IDatabaseViewDesc>' is not assignable to type 'IViewDescription'.
    Property 'idType' is optional in type 'Readonly<IDatabaseViewDesc>' but required in type 'IViewDescription'.

@lehnerchristian
Copy link

in which project do you get the error? I've checked your branch out in my local version of Ordino and I don't get any compile errors

@keckelt
Copy link
Contributor Author

keckelt commented Sep 19, 2019

In Ordino, I have errors in the subclasses of ARankingView like: https://github.com/Caleydo/tdp_publicdb/blob/master/src/views/ACombinedDependentTable.ts#L77

I think we should change the return type of loadColumnDesc, but don't know how much work that would be (e.g. to IServerCoumn[]), or if that is better in another PR.

@keckelt
Copy link
Contributor Author

keckelt commented Sep 20, 2019

Return of /api/tdp/db/<db>/<view>/desc before and after
Old:

{
  "idType": null,
  "columns": []
}

New:

{
  "description": "",
  "arguments": [
    "query",
    "query",
    "species",
    "column",
    "limit",
    "offset"
  ],
  "query": "SELECT s as id, s as text FROM (SELECT distinct {column} AS s FROM tissue.tdp_tissue WHERE LOWER({column}) LIKE :query AND species = :species) ORDER BY {column} ASC LIMIT {limit} OFFSET {offset}",
  "type": "helper",
  "columns": [],
  "name": "tissue_unique"
}

@keckelt
Copy link
Contributor Author

keckelt commented Sep 20, 2019

Hi @lehnerchristian, I decided to not make the IViewDescription properties optional because they are always returned by the API (idType can be null/the array can be empty though).

@lehnerchristian
Copy link

ok, thank you. I'll have a look

@lehnerchristian
Copy link

sorry, I'll need to have a look at all projects, because this PR has some breaking changes. I talked to @thinkh and we're checking how we release it: a new major version + probably renaming of IViewDescription to IViewDesc

@lehnerchristian
Copy link

I've checked this PR with several projects especially if there are some RankingViews returning only columns without IDTypes . I'll need to check some other projects, which I'm not involved in with some colleagues tomorrow and then I can hopefully merge this PR

src/rest.ts Outdated Show resolved Hide resolved
src/rest.ts Outdated Show resolved Hide resolved
@lehnerchristian
Copy link

@dg-datavisyn will test this with his projects, too and then this change will be in the next release in 2 weeks

in the meantime we'll merge this into develop after the release we'll create now.

thank you for the changes and sorry that it took me so long, @keckelt 😉

Copy link

@lehnerchristian lehnerchristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this PR with another project and it works for me. @thinkh , there are some comments to your comments

@lehnerchristian
Copy link

as soon as it's renamed as @thinkh suggested we'll merge it

@lehnerchristian
Copy link

@keckelt , can you rename the interfaces as @thinkh suggested?

@keckelt
Copy link
Contributor Author

keckelt commented Oct 16, 2019

@lehnerchristian Done 🙂

@lehnerchristian lehnerchristian merged commit 1afc4f8 into develop Oct 21, 2019
@lehnerchristian lehnerchristian deleted the keckelt/idType-viewDesc branch October 21, 2019 13:21
@lehnerchristian lehnerchristian mentioned this pull request Nov 7, 2019
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants