dbeaver/pro#9652 refactor ref api#4412
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
| Complexity | 1 medium |
🟢 Metrics 80 complexity · -5 duplication
Metric Results Complexity 80 Duplication -5
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.
HocKu7
force-pushed
the
dbeaver/pro#9652-refactor-ref-api
branch
from
64dc0dd to
ea85e9c
Compare
| return where; | ||
| } | ||
|
|
||
| public boolean isAnyConstraint() { |
There was a problem hiding this comment.
What is this? Add javadoc pls
|
|
||
| type SQLResultReference @since(version: "26.1.1") { | ||
| "True for reverse references (incoming FKs)" | ||
| isReference: Boolean! |
| } | ||
|
|
||
| "Pair (source-row column index, target-entity column name) used to build a navigation filter" | ||
| type SQLReferenceColumnMapping @since(version: "26.1.1") { |
| state.association = defaultAssociation.associationName; | ||
| } | ||
|
|
||
| //@TODO Find a better way to check whether model is ready to be used |
There was a problem hiding this comment.
is there a separate task or will be fixed here?
|
|
||
| function openAssociation() { | ||
| if (!currentAssociation?.targetNodePath) { | ||
| throw new Error('Target node path is not defined'); |
There was a problem hiding this comment.
this things like throw new Error on layout level usually breaks a whole layout. did you test this case? what will happen? I suppose like notification or StatusMessage is more than enough in most cases
There was a problem hiding this comment.
This case is never going to happen as we render open button only if currentAssociation is presented
There was a problem hiding this comment.
I'm speaking from experience here. All of these cases we had this throw new Error() we had and which are never supposed to happen usually happens. Then we create a ticket for that and fix the issue with replacing this error with:
- early return
- log or notification or status message
I personally fixed at least 2-3 bugs like this during this year. So all I'm saying is after few iterations with this feature, this may happen. Right now we can fix it quite cheap since we noticed it pretty early
| {currentAssociation.targetNodePath && ( | ||
| <Button size="small" variant="secondary" onClick={openAssociation}> | ||
| {translate('ui_open')} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
lets always show the button so the interface don't jump
but disable it if no target node path or something is loading
There was a problem hiding this comment.
It would be strange, user will see a button but would not be able to click it. TargetNodePath should be provided as we are fetching result based on that
There was a problem hiding this comment.
okay. my concern here is so user can't press it if something is loading and that there are not blinking interface
if you think its fine I'm good with that
| asyncTaskInfoService: AsyncTaskInfoService, | ||
| ) { | ||
| super(serviceProvider, graphQLService, asyncTaskInfoService, connectionExecutionContextService); | ||
| this.setFeature(DatabaseDataFeature.References); |
There was a problem hiding this comment.
i can see that it is not reused any where (only few places to show/hide the feature itself). this means reference panel table cells probably has no actions can be applied via context menu or shortcuts
is this expected?
since it is ResultSet I would also try to add References in the implementations of ResultSet with allowedFeatures arrays. and check if features works correct here. if something is broken we can remove it or comment + add the ticket to fix it
the point is to add a functionality from the start what we can afford, without going too deep into it right now
There was a problem hiding this comment.
I dont follow the logic, we set feature here so we dont need to check instance of source, the same way we do for grouping panel
There was a problem hiding this comment.
okay lets leave DatabaseDataFeature.References it is fine and expected
what about context menu and shortcuts? can we add something so its not empty? like generate sql or sorting if it is possible
There was a problem hiding this comment.
We have sorting, I checked grouping panel and we have the same actions there. Probably we should create a ticket and add more actions to grouping panel and references panel
There was a problem hiding this comment.
okay lets go with the ticket then
| state.association = defaultAssociation.associationName; | ||
| } | ||
|
|
||
| //@TODO Find a better way to check whether model is ready to be used |
6 participants
Closes https://github.com/dbeaver/pro/issues/9652