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

SAP Hana dialect #215

Merged
merged 35 commits into from May 11, 2019

Conversation

3 participants
@ariel-bentu
Copy link
Contributor

commented May 7, 2019

Supporting SAP HANA Dialect.

At this point only tables.

TODO:

  • Support functions and Views
  • Docs

Related to #212
Related to #20

Ariel Bentolila added some commits May 6, 2019

@project-bot project-bot bot added this to In progress in Kanban May 7, 2019

@mtxr mtxr self-requested a review May 7, 2019

import queries from './queries';
import GenericDialect from '@sqltools/core/dialect/generic';
import { DatabaseInterface } from '@sqltools/core/plugin-api';
// Cannot simply use hanaClient from '@sap/hana-client', because webpack is moving it away from its native libs.

This comment has been minimized.

Copy link
@mtxr

mtxr May 7, 2019

Owner

Take a look at SQLite and Oracle implementation. I believe that this case would be better if handled by the dependencyManager like SQLite and Oracle does

This comment has been minimized.

Copy link
@ariel-bentu

ariel-bentu May 7, 2019

Author Contributor

done

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 7, 2019

@ariel-bentu nice work! That was fast!

@mtxr mtxr changed the title Saphana dialect WIP: SAP Hana dialect May 7, 2019

ariel-bentu and others added some commits May 7, 2019

Ariel Bentolila
@codecov

This comment has been minimized.

Copy link

commented May 7, 2019

Codecov Report

Merging #215 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #215      +/-   ##
=========================================
- Coverage    4.15%   4.15%   -0.01%     
=========================================
  Files          82      83       +1     
  Lines        2622    2626       +4     
  Branches      514     515       +1     
=========================================
  Hits          109     109              
- Misses       2504    2508       +4     
  Partials        9       9
Impacted Files Coverage Δ
packages/core/dialect/generic.ts 0% <ø> (ø) ⬆️
...kages/plugins/connection-manager/explorer/index.ts 0% <ø> (ø) ⬆️
packages/core/dialect/saphana/queries.ts 0% <0%> (ø)
...ages/plugins/dependency-manager/language-server.ts 0% <0%> (ø) ⬆️
packages/core/dialect/index.ts 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc93627...db602f9. Read the comment docs.

@mtxr mtxr self-requested a review May 7, 2019

@mtxr
Copy link
Owner

left a comment

@ariel-bentu awesome! You are doing a great job, thanks!

I just commented some places that we wont need or is just a small change. I'm really excited with you PR.

Just explaining a bit about the deps, you can add@types/saphana (if exists and if you want) instead of the driver on package.json files.

On first usage, user will be requested to install it, that helps on packing because it allows us to pack for multi platforms and let node handle deps.

Once you finish here, let me know so I can merge.

Dont forget to write the docs for connecting.

Again, thank you very much! I'll reach you back as soon as I have updates on licensing.

Show resolved Hide resolved .test-database/.vscode/settings.json
Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved packages/core/dialect/saphana/index.ts Outdated
Show resolved Hide resolved packages/core/dialect/saphana/index.ts Outdated
Show resolved Hide resolved packages/core/dialect/saphana/index.ts Outdated
Show resolved Hide resolved packages/plugins/package.json Outdated
Show resolved Hide resolved packages/core/package.json Outdated
Show resolved Hide resolved packages/language-server/webpack.config.js Outdated
Show resolved Hide resolved packages/language-server/webpack.config.js Outdated
@ariel-bentu

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

On first usage, user will be requested to install it, that helps on packing because it allows us to pack for multi platforms and let node handle deps.

is there a way to advice the dependency manager that this package is?
The package is in a public npm repo maintained by SAP at https://npm.sap.com

Instructions from its site is:
npm config set @SAP:registry https://npm.sap.com
npm install @sap/hana-client

In addition, why is it not finding the module, if I previouslly installed it globally (manually ran npm install -g ... )?
Thanks

Ariel Bentolila and others added some commits May 8, 2019

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

On first usage, user will be requested to install it, that helps on packing because it allows us to pack for multi platforms and let node handle deps.

is there a way to advice the dependency manager that this package is?
The package is in a public npm repo maintained by SAP at https://npm.sap.com

Instructions from its site is:
npm config set @SAP:registry https://npm.sap.com
npm install @sap/hana-client

In addition, why is it not finding the module, if I previouslly installed it globally (manually ran npm install -g ... )?
Thanks

No, we don't have it for now. But I'll add in you PR, ok? @ariel-bentu

@ariel-bentu

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

go ahead and merge.
any idea why global on npm is not recognized?

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

About the modules, for now it's better to install the module inside of the extension folder because in global install users usually have the latest version, and for the extension we are going to fix it in a tested version.

I'll send a review. It's 2 questions only. See below:

mtxr added some commits May 11, 2019

@@ -0,0 +1,5 @@
module.exports = function (ns){

This comment has been minimized.

Copy link
@mtxr

mtxr May 11, 2019

Owner

Is this file mandatory? If not, please, consider removing

This comment has been minimized.

Copy link
@mtxr

mtxr May 11, 2019

Owner

Also, please remove that DS_store file and add it to .gitignore on project root

This comment has been minimized.

Copy link
@ariel-bentu

ariel-bentu May 11, 2019

Author Contributor

no actually this file is left over before move to package manager

Show resolved Hide resolved packages/core/dialect/saphana/index.ts
@mtxr

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

I'll add the registry specification and reach you back.

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

@ariel-bentu https://github.com/ariel-bentu/vscode-sqltools/pull/1/files please see and test it, but I believe after this we can merge it on master

Merge pull request #1 from mtxr/saphanaDialect
saphanaDialect: Add support for extra args for npm
@mtxr

This comment has been minimized.

Copy link
Owner

commented May 11, 2019

Is it fully working? @ariel-bentu

@ariel-bentu

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Is it fully working? @ariel-bentu

I sent you by mail, you need to change to:
args: ['--@SAP:registry=https://npm.sap.com']

@mtxr

mtxr approved these changes May 11, 2019

@mtxr mtxr merged commit cbc40c0 into mtxr:master May 11, 2019

2 of 3 checks passed

WIP Title contains "WIP"
Details
Travis CI - Pull Request Build Passed
Details
codeclimate All good!
Details

Kanban automation moved this from In progress to To be released May 11, 2019

@ariel-bentu

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Regarding the -g option: using a globally installed package instead of requiring installation:

I found that npm has the command link:

npm link '@sap/hana-client'

It would be nice to first try to link, and if fails (exit code != 0) fall back to installing it. what do you think?

@mtxr mtxr changed the title WIP: SAP Hana dialect SAP Hana dialect May 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.