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

feat:support cypher parameter #72

Merged
merged 3 commits into from
Dec 30, 2021
Merged

Conversation

hetao92
Copy link
Contributor

@hetao92 hetao92 commented Dec 28, 2021

No description provided.

@hetao92 hetao92 requested review from nianiaJR and veezhang December 28, 2021 09:37
@@ -287,26 +287,29 @@ func Disconnect(nsid string) {
pool.Disconnect(nsid)
}

func Execute(nsid string, gql string) (result ExecuteResult, err error) {
func Execute(nsid string, gql string, paramList common.ParameterList) (result ExecuteResult, cmdResult string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cmdResult? what's it like?

Comment on lines 281 to 356
if len(request.ParamList) > 0 {
err = executeCmd(request.ParamList, &connection.parameterMap)
if err != nil {
cmdResult = err.Error()
} else {
cmdResult = SetParamsSuccess
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's like that the cmdResult is not necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's error, just return error. The channel response result just return nil, error enough

refactor: remove vscode file

update: fix typo value

mod: update local cmd

mod: update params

mod: support params

mod: add type
nianiaJR
nianiaJR previously approved these changes Dec 29, 2021
Copy link
Contributor

@nianiaJR nianiaJR left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -287,26 +287,30 @@ func Disconnect(nsid string) {
pool.Disconnect(nsid)
}

func Execute(nsid string, gql string) (result ExecuteResult, err error) {
func Execute(nsid string, gql string, paramList common.ParameterList) (result ExecuteResult, params common.ParameterMap, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not put params into ExecuteResult?

Comment on lines 97 to 101
if len(paramsMap) == 0 {
res.Params = nil
} else {
res.Params = paramsMap
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value of res.Params is nil, need not to assign with nil.

if len(paramsMap) > 0 {
    res.Params = paramsMap
}

@@ -12,11 +15,20 @@ import (

uuid "github.com/satori/go.uuid"
nebula "github.com/vesoft-inc/nebula-go/v2"
nebulaType "github.com/vesoft-inc/nebula-go/v2/nebula"
Copy link
Contributor

@veezhang veezhang Dec 29, 2021

Choose a reason for hiding this comment

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

The pkg name alias seems that they are all lowercase, and I haven't seen any uppercase ones.
Use nebulatype instead?

Comment on lines 224 to 227
if reg == nil {
err = errors.New("invalid regular expression")
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The reg cannot be nil.

*/
if len(res) != 2 {
return
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

useless else.

Comment on lines 183 to 186
{
localCmd = Param
args = []string{plain}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

useless { and }.

Copy link
Contributor

@veezhang veezhang left a comment

Choose a reason for hiding this comment

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

LGTM

@hetao92 hetao92 merged commit e42e3b0 into vesoft-inc:master Dec 30, 2021
@hetao92 hetao92 deleted the hetao-dev branch December 30, 2021 06:00
@Sophie-Xie Sophie-Xie added this to the v3.0.0 milestone Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants