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

Refactor #213

Merged
merged 1 commit into from
May 19, 2022
Merged

Refactor #213

merged 1 commit into from
May 19, 2022

Conversation

veeding
Copy link
Contributor

@veeding veeding commented May 13, 2022

complete import task and pass all test

@CLAassistant
Copy link

CLAassistant commented May 13, 2022

CLA assistant check
All committers have signed the CLA.

@veeding veeding force-pushed the refactor branch 2 times, most recently from 416ca11 to 2ace00c Compare May 17, 2022 01:15
server-v2/api/studio/restapi/import.api Show resolved Hide resolved
"github.com/vesoft-inc/nebula-studio/server/api/studio/internal/service"
"net/http"
Copy link
Contributor

Choose a reason for hiding this comment

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

Forget to make check?

@@ -10,6 +10,7 @@ import (
"github.com/saintfish/chardet"
"github.com/vesoft-inc/nebula-studio/server/api/studio/internal/svc"
"github.com/vesoft-inc/nebula-studio/server/api/studio/internal/types"
Config "github.com/vesoft-inc/nebula-studio/server/api/studio/pkg/config"
Copy link
Contributor

Choose a reason for hiding this comment

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

The alias of pkg usually lowercase. And this pkg will be removed.

server-v2/api/studio/internal/service/import.go Outdated Show resolved Hide resolved
Comment on lines 50 to 53
func WithSessionMessage(err error, formatWithArgs ...interface{}) error {
ErrSessionWithMessage := newErrCode(CCUnauthorized, PlatformCode, 1, fmt.Sprintf("ErrSession::%s", err.Error()))
return WithCode(ErrSessionWithMessage, err, formatWithArgs...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Only very general ones are put here, this is not recommended to be put here, this file should not be easily changed.

You just need to define the new ErrCode after line 12 // Define you error code here.

"wmv": "video/x-ms-wmv",
"avi": "video/x-msvideo",
// dynamicly add query params to the request
func AddQueryParams(r *http.Request, params map[string]string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Modified the original query parameter, it seems strange, is there a better way?

post /api/file/upload
@doc "delete file"
@handler FileDestroy
delete /api/file/:name(FileDestroyRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about /api/files/:name ?

delete /api/file/:name(FileDestroyRequest)
@doc "preview file"
@handler FilesIndex
get /api/file returns(FilesIndexData)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about '/api/files`?

server-v2/api/studio/restapi/file.api Outdated Show resolved Hide resolved
@veezhang
Copy link
Contributor

please resolve conflicts.

@huaxiabuluo huaxiabuluo force-pushed the refactor branch 2 times, most recently from 21ef801 to 85ea357 Compare May 17, 2022 08:24
get /api/import-tasks/stop/:id(StopImportTaskRequest)
@doc "Download logs"
@handler DownloadLogs
get /api/import-tasks/download/logs/:id(DownloadLogsRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about /api/import-tasks/:id/download-logs ?

get /api/import-tasks/download/logs/:id(DownloadLogsRequest)
@doc "Download Config"
@handler DownloadConfig
get /api/import-tasks/download/config/:id(DownloadConfigsRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about /api/import-tasks/:id/download-config?

if strings.HasPrefix(r.URL.Path, "/api/files") {
return false
}
if strings.HasPrefix(r.URL.Path, "/api/import-tasks/download") {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several places with this logic, can you encapsulated into a function?

@veeding veeding force-pushed the refactor branch 4 times, most recently from a0deae0 to d208aa2 Compare May 18, 2022 10:53
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

@@ -0,0 +1,29 @@
package common
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a more appropriate file name.

modify upload and download

mody: go mod

refine the code

update: make check

motify api

modify return values

rename

fix
@huaxiabuluo huaxiabuluo merged commit 7ffe590 into vesoft-inc:refactor May 19, 2022
hetao92 pushed a commit that referenced this pull request May 26, 2022
modify upload and download
mody: go mod
refine the code
update: make check
modify return values
hetao92 pushed a commit that referenced this pull request May 26, 2022
feat: mime && connectdb

store

u

文件上传、删除、预览功能实现 (#206)

feat: middleware (#208)

* feat: middleware
* feat: dynamicly add query params func
* feat: common params
* update: go mod tidy
* feat: batchExec

complete file and import task (#213)

modify upload and download
mody: go mod
refine the code
update: make check
modify return values

Refactor (#219)

* update: server-v2
* updata: format

feat: context auth (#220)

update: export module (#222)

feat: export RegisterHandlers (#223)

update: gitignore (#224)

fix: fix service init crash (#225)

update: WithErrorMessage (#226)

fix import task (#227)

fix the type conveersion problem

fix: fix import & file model (#229)
huaxiabuluo added a commit that referenced this pull request May 26, 2022
feat: mime && connectdb
feat: 文件上传、删除、预览功能实现 (#206)
feat: middleware (#208)
* feat: middleware
* feat: dynamicly add query params func
* feat: common params
* update: go mod tidy
* feat: batchExec

complete file and import task (#213)

modify upload and download
mody: go mod
refine the code
update: make check
modify return values

Refactor (#219)

* update: server-v2
* updata: format

feat: context auth (#220)

update: export module (#222)

feat: export RegisterHandlers (#223)

update: gitignore (#224)

fix: fix service init crash (#225)

update: WithErrorMessage (#226)

fix import task (#227)

fix the type conveersion problem

fix: fix import & file model (#229)

Co-authored-by: Bruce <[email protected]>
veeding added a commit to veeding/nebula-studio that referenced this pull request Jun 14, 2022
modify upload and download
mody: go mod
refine the code
update: make check
modify return values
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.

4 participants