Skip to content

Conversation

@Donya-qlik
Copy link
Contributor

@Donya-qlik Donya-qlik commented Apr 6, 2025

Motivation

Moving generic hypercube functions from sense-client to nebula.js - Part01.
At this PR only the functions that are required to display the fields in property-panel are considered. The rest of the functions will be moved to this repo in the coming PRs.

The source files for these functions are in sense-client (data-property-handler.ts & hypercube-handler.js)

Required Flag: NEBULA_DATA_HANDLERS

Requirements checklist

  • Api specification
    • Ran yarn spec
      • No changes OR API changes has been formally approved
  • Unit/Component test coverage
  • Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

  • Add code reviewers, for example @qlik-oss/nebula-core

@Donya-qlik Donya-qlik changed the title feat: add hypercube generic functions to nebuls.js, part01 refactor: add hypercube generic functions to nebuls.js, part01 Apr 6, 2025
@Donya-qlik Donya-qlik requested a review from Copilot April 6, 2025 23:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • apis/nucleus/package.json: Language not supported
  • package.json: Language not supported

@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from d12ed36 to d701919 Compare April 7, 2025 07:22
@Donya-qlik Donya-qlik requested a review from Copilot April 7, 2025 07:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 11 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • apis/nucleus/package.json: Language not supported
  • apis/stardust/api-spec/spec.json: Language not supported
  • package.json: Language not supported
Comments suppressed due to low confidence (2)

apis/nucleus/src/utils/handlers/hypercube-handlers.js:23

  • [nitpick] The inline ternary inside the template literal is redundant. Consider simplifying the expression to use ${this.path}.qHyperCubeDef when this.path exists for improved clarity.
      ? utils.getValue(properties, `${this.path ? `${this.path}.` : ''}qHyperCubeDef`)

apis/conversion/src/array-util.js:77

  • [nitpick] Using Array.isArray(array) is generally preferred over instanceof Array for broad reliability across different execution contexts.
  if (!(array instanceof Array)) {

@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch 3 times, most recently from eaa2870 to 0b1317b Compare April 7, 2025 08:02
@Donya-qlik Donya-qlik requested a review from Caele April 8, 2025 07:23
@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from d6a9130 to 1379f85 Compare April 9, 2025 23:04
@Donya-qlik Donya-qlik requested a review from Caele April 10, 2025 13:50
@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch 2 times, most recently from 222061c to 96e0ca5 Compare April 10, 2025 17:48
@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from c62f495 to 7ee1ab0 Compare April 11, 2025 09:07
@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from 7ee1ab0 to 430b60c Compare April 11, 2025 09:09
@Donya-qlik Donya-qlik requested a review from Caele April 13, 2025 22:35
@Donya-qlik Donya-qlik requested a review from Copilot April 14, 2025 08:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

@Donya-qlik Donya-qlik requested a review from Caele April 14, 2025 22:18
@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from fe9e437 to 63b983e Compare April 16, 2025 10:10
@Donya-qlik
Copy link
Contributor Author

Donya-qlik commented Apr 16, 2025

@Caele I added the flag so it works only when the flag is enabled. Do you think the PR is ready to merge?

Btw, I disabled checking Eslint for some imports from the other packages for now, since you mentioned we prefer to not to add other packages dependencies. But we should probably come up with a better solution for that, maybe moving those util functions to the parent package so they can be easily called in different packages without necessity to add a specific dependency?
If it makes sense, I can do that in a separated PR.

@Donya-qlik Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch from 63b983e to c56321c Compare April 16, 2025 11:11
Copy link
Collaborator

@Caele Caele left a comment

Choose a reason for hiding this comment

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

Can go ahead with this now. Not sure about the extra line in the spec file. but thats probably an auto fix thing.

@enell enell requested a review from a team April 22, 2025 07:18
@Donya-qlik Donya-qlik merged commit 7603c9b into main Apr 22, 2025
10 checks passed
@Donya-qlik Donya-qlik deleted the dmh/VNA-46-move-hc-generic-fn branch April 22, 2025 08:17
@Donya-qlik Donya-qlik changed the title refactor: add hypercube generic functions to nebuls.js, part01 chore: add hypercube generic functions to nebuls.js, part01 Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants