dbeaver/pro#7128 Remove deprecated API#3820
Conversation
serge-rider
dismissed stale reviews from alexander-skoblikov and Wroud
via
479baa7
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 123 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
devnaumov
requested review from
HocKu7,
SychevAndrey,
sergeyteleshev and
yagudin10
| const node: NavNode = { | ||
| uri: '', | ||
| folder: true, | ||
| hasChildren: false, | ||
| inline: false, | ||
| navigable: false, | ||
| filtered: false, | ||
| objectFeatures: [], | ||
| nodeType: NAV_NODE_TYPE_FOLDER, | ||
| }; |
There was a problem hiding this comment.
this is a bad pattern to just update the object. so lets create a mock function which creates new object each time based on args we provide
| export function getFolderPath(node: NavNode): string { | ||
| return getFolderPathWithProjectId(node).split('/').slice(1).join('/'); | ||
| } |
There was a problem hiding this comment.
unit tests would be nice here
|
|
||
| export function getFolderPathWithProjectId(folderId: string): string { | ||
| if (!isFolderNodeId(folderId)) { | ||
| export function getFolderPathWithProjectId(node: NavNode): string { |
| getPlainPath(nodeId: string): string { | ||
| return nodeId.startsWith(NODE_PATH_PREFIX) ? nodeId.slice(NODE_PATH_PREFIX.length) : nodeId; | ||
| }, |
There was a problem hiding this comment.
this one can be covered with unit tests as well
There was a problem hiding this comment.
Yes but lets do it a little bit later
There was a problem hiding this comment.
I think the value here is more than time to spent to generate these tests
if some bugs appear we will definitely know these parts are out of suspects
| const parts = getPathParts(path); | ||
|
|
||
| return parts.map((_, i, array) => createPath(...array.slice(0, i))).filter(parent => parent !== path); | ||
| return parts.map((_, i, array) => createPath(...array.slice(0, i))).filter(parent => parent !== path && parent !== ''); |
There was a problem hiding this comment.
could you specify why this changed?
There was a problem hiding this comment.
createPath might return empty string, previously it was valid parent but now its not
| const parents = data.nodePaths.map(nodeId => { | ||
| const parents = getFolderNodeParents(nodeId); | ||
|
|
||
| const parents = this.navNodeInfoResource.getParents(nodeId); |
There was a problem hiding this comment.
am I understand it right?
now folders has no prefix "folder". it has prefix "node". and we got lost an ability to define wether its folder or not by uri path?
I see that we can define it by node type but if possible it would be nice to define it also by path its quite useful
There was a problem hiding this comment.
Yes, now folder is just a node in terms of path structure. node://{project}/datasource/{...folders}/{connection}/{...}
| return nodeId.startsWith('folder://'); | ||
| import { NAV_NODE_TYPE_FOLDER, type NavNode } from '@cloudbeaver/core-navigation-tree'; | ||
|
|
||
| export function isFolderNodeId(node: NavNode): boolean { |
| } | ||
| return true; | ||
|
|
||
| return NodeManagerUtils.isDatabaseObject(node.uri); |
There was a problem hiding this comment.
To exclude extra nodes that are now returning with datasources
| const node: NavNode = { | ||
| uri: '', | ||
| folder: true, | ||
| hasChildren: false, | ||
| inline: false, | ||
| navigable: false, | ||
| filtered: false, | ||
| objectFeatures: [], | ||
| nodeType: NAV_NODE_TYPE_FOLDER, | ||
| }; | ||
|
|
||
| test('should extract projectId and folderId from a valid nodeId', () => { | ||
| const id = 'node://u_cbadmin/datasources/QAtestRegress'; | ||
| node.uri = id; | ||
| expect(getConnectionFolderIdFromNodeId(node)).toEqual({ projectId: 'u_cbadmin', folderId: 'QAtestRegress' }); | ||
| }); | ||
|
|
||
| test('should return undefined for non-folder ids', () => { | ||
| const id = 'ext://resources/g_GlobalConfiguration/Aleksandr'; | ||
| expect(getConnectionFolderIdFromNodeId(id)).toEqual(undefined); | ||
| const id = 'node://u_cbadmin/datasources/QAtestRegress'; | ||
|
|
||
| node.uri = id; | ||
| node.folder = false; | ||
| delete node.nodeType; |
There was a problem hiding this comment.
instead of object mutation maybe it's better to use simple factory here and create needed node instance in each case? It will make test more isolated.
| return nodeId.startsWith('folder://'); | ||
| import { NAV_NODE_TYPE_FOLDER, type NavNode } from '@cloudbeaver/core-navigation-tree'; | ||
|
|
||
| export function isFolderNodeId(node: NavNode): boolean { |
There was a problem hiding this comment.
I would rename this fn to smth like isNodeAFolder
We do not operate with any node id anymore
| @Property | ||
| @Deprecated(forRemoval = true) | ||
| public String getId() { | ||
| return node.getNodeItemPath(); | ||
| return node.getNodeUri(); | ||
| } | ||
|
|
…er/cloudbeaver into 7128-remove-deprecated-api
serge-rider
dismissed stale reviews from sergeyteleshev and SychevAndrey
via
63ef6c3
8 participants
No description provided.