-
Notifications
You must be signed in to change notification settings - Fork 51
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
remove underscore from variable names #684
remove underscore from variable names #684
Conversation
@computerphilosopher 기여 감사합니다 ~~ !! ^^ 기여자께서 camel case 로 일관성을 맞춰주셨습니다. 일부 변수명은 약어(보통 대문자로 가이드 되는..)가 함께 포함되어 있어서, 예를 들면,
참고: https://github.com/golang/go/wiki/CodeReviewComments#initialisms
@jihoon-seo @hermitkim1 함께 봐주시면 좋을 것 같습니다.. ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher 기여 감사합니다 😊
이 리뷰는 @computerphilosopher 님께 즉각적인 후속조치를 요청하는 리뷰는 아니며
변수 명칭 등의 스타일에 대한 의견을 내는 리뷰로 보시면 되겠습니다. 😊
@@ -374,7 +374,7 @@ func (s *MCISService) RecommendMcis(ctx context.Context, req *pb.McisRecommendCr | |||
} | |||
|
|||
content := rest_mcis.RestPostMcisRecommendResponse{} | |||
content.Vm_recommend = result | |||
content.VmRecommend = result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seokho-son
https://github.com/golang/go/wiki/CodeReviewComments#initialisms 에 따라
VMRecommend
가 적절하려나요 ㅎㅎ
또는, VMRecommendation
등등..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecommendedVM
은 어떠신지요? ^^
var SpiderRestUrl string | ||
var DragonflyRestUrl string | ||
var DBUrl string | ||
var DBDatabase string | ||
var DBUser string | ||
var DBPassword string | ||
var AutocontrolDurationMs string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 변수들은, CB-Tumblebug에서 사용되고 있는 Environment variable 들과 대응되는 Go variable 들이라서
Go variable 이름을 Environment variable 과 동일하게 맞춘 측면이 있기는 합니다.. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
환경변수와 관련된 string 변수를 struct로 묶어서 환경변수와 관련 있다는 사실을 드러내면 어떨까요?
type EnvironmentVariable struct {
SpiderRestUrl string
DragonflyRestUrl string
DBUrl string
DBDatabase string
DBUser string
DBPassword string
AutocontrolDurationMs string
}
var EnvVar EnvironmentVariable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher 좋은 아이디어 같습니다!! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher 좋은 방법이네요! ^^
해당 사항은 이번 PR 처리 후에,
신규 이슈 및 PR을 통해서 업데이트하면 더욱 좋을 것 같습니다. 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
머지된 후 다시 이슈 만들겠습니다.
@@ -154,7 +154,7 @@ func DelResource(nsId string, resourceType string, resourceId string, forceFlag | |||
} | |||
|
|||
//delete related recommend spec | |||
err = DelRecommendSpec(nsId, resourceId, content.Num_vCPU, content.Mem_GiB, content.Storage_GiB) | |||
err = DelRecommendSpec(nsId, resourceId, content.NumvCPU, content.MemGiB, content.StorageGiB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumvCPU
vs. NumVCPU
vs. something else? 😊
src/core/mcir/spec.go
Outdated
OsType string `json:"os_type"` | ||
NumvCPU uint16 `json:"num_vCPU"` | ||
NumCore uint16 `json:"num_core"` | ||
MemGiB uint16 `json:"mem_GiB"` | ||
StorageGiB uint32 `json:"storage_GiB"` | ||
Description string `json:"description"` | ||
Cost_per_hour float32 `json:"cost_per_hour"` | ||
Num_storage uint8 `json:"num_storage"` | ||
Max_num_storage uint8 `json:"max_num_storage"` | ||
Max_total_storage_TiB uint16 `json:"max_total_storage_TiB"` | ||
Net_bw_Gbps uint16 `json:"net_bw_Gbps"` | ||
Ebs_bw_Mbps uint32 `json:"ebs_bw_Mbps"` | ||
Gpu_model string `json:"gpu_model"` | ||
Num_gpu uint8 `json:"num_gpu"` | ||
Gpumem_GiB uint16 `json:"gpumem_GiB"` | ||
Gpu_p2p string `json:"gpu_p2p"` | ||
CostPerHour float32 `json:"cost_per_hour"` | ||
NumStorage uint8 `json:"num_storage"` | ||
MaxNumStorage uint8 `json:"max_num_storage"` | ||
MaxTotalStorageTiB uint16 `json:"max_total_storage_TiB"` | ||
NetBwGbps uint16 `json:"net_bw_Gbps"` | ||
EbsBwMbps uint32 `json:"ebs_bw_Mbps"` | ||
GpuModel string `json:"gpu_model"` | ||
NumGpu uint8 `json:"num_gpu"` | ||
GpuMemGiB uint16 `json:"gpumem_GiB"` | ||
GpuP2p string `json:"gpu_p2p"` | ||
OrderInFilteredResult uint16 `json:"orderInFilteredResult"` | ||
EvaluationStatus string `json:"evaluationStatus"` | ||
EvaluationScore_01 float32 `json:"evaluationScore_01"` | ||
EvaluationScore_02 float32 `json:"evaluationScore_02"` | ||
EvaluationScore_03 float32 `json:"evaluationScore_03"` | ||
EvaluationScore_04 float32 `json:"evaluationScore_04"` | ||
EvaluationScore_05 float32 `json:"evaluationScore_05"` | ||
EvaluationScore_06 float32 `json:"evaluationScore_06"` | ||
EvaluationScore_07 float32 `json:"evaluationScore_07"` | ||
EvaluationScore_08 float32 `json:"evaluationScore_08"` | ||
EvaluationScore_09 float32 `json:"evaluationScore_09"` | ||
EvaluationScore_10 float32 `json:"evaluationScore_10"` | ||
EvaluationScore01 float32 `json:"evaluationScore_01"` | ||
EvaluationScore02 float32 `json:"evaluationScore_02"` | ||
EvaluationScore03 float32 `json:"evaluationScore_03"` | ||
EvaluationScore04 float32 `json:"evaluationScore_04"` | ||
EvaluationScore05 float32 `json:"evaluationScore_05"` | ||
EvaluationScore06 float32 `json:"evaluationScore_06"` | ||
EvaluationScore07 float32 `json:"evaluationScore_07"` | ||
EvaluationScore08 float32 `json:"evaluationScore_08"` | ||
EvaluationScore09 float32 `json:"evaluationScore_09"` | ||
EvaluationScore10 float32 `json:"evaluationScore_10"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(CC: @seokho-son )
Go 변수 이름과 JSON 필드 이름을 꼭 일치시켜야 하는 것은 아니지만,
현재 JSON 필드 쪽도 camelCase 와 snake_case 가 혼재되어 있기 때문에
camelCase 로 통일을 시키면 좋을 것 같습니다.
다만, 이렇게 하면 REST API에 변화가 생기기 때문에
CB-Ladybug, cb-webtool 도 CB-Tumblebug 변경사항에 대응해야 하며
CB-Tumblebug의 gRPC 코드도 업데이트가 필요할 것입니다. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 감사합니다. 당연히 Go 변수 이름과 JSON 필드 이름에도 반영이 필요해 보입니다.
우선 해당 PR에서는 JSON 필드 업데이트 및 REST API 문서에 반영까지 요청해보면 어떨까 싶습니다. (변수명 컨벤션을 맞춘 다음)
API 변경이 다른 FW에 영향을 주는 부분은 어쩔 수 없는 부분인 것 같습니다.
이번 기회에 snakeCase는 모두 정리해보아요! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jihoon-seo Go변수명과 Json 필드명을 완전히 같게 만드는 것은 어떻게 생각하시나요? 현재는 Go변수명은 첫글자를 대문자로 하고 있습니다.
For discussion, https://github.com/golang/go/wiki/CodeReviewComments#initialisms
https://github.com/golang/lint/blob/c5fb716d6688a859aae56d26d3e6070808df29f7/lint.go#L742
|
@computerphilosopher 아직 CB-TB에 네이밍 컨벤션이 아직 명확하지 않아서, PR 처리가 지연되고 있습니다. 논의 주요 포인트
참고: #689 |
src/core/mcir/spec.go
Outdated
//sqlQuery += " AND `evaluationScore_01`>=" + fmt.Sprintf("%.6f", filter.EvaluationScore_01.Min) | ||
sqlQuery = sqlQuery.And("EvaluationScore_01 >= ?", filter.EvaluationScore_01.Min) | ||
sqlQuery = sqlQuery.And("EvaluationScore_01 >= ?", filter.EvaluationScore01.Min) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sqlQuery = sqlQuery.And("EvaluationScore_01 >= ?", filter.EvaluationScore01.Min) | |
sqlQuery = sqlQuery.And("EvaluationScore01 >= ?", filter.EvaluationScore01.Min) |
누락된 항목으로 보입니다.
src/core/mcir/spec.go
Outdated
//sqlQuery += " AND `evaluationScore_01`<=" + fmt.Sprintf("%.6f", filter.EvaluationScore_01.Max) | ||
sqlQuery = sqlQuery.And("EvaluationScore_01 <= ?", filter.EvaluationScore_01.Max) | ||
sqlQuery = sqlQuery.And("EvaluationScore_01 <= ?", filter.EvaluationScore01.Max) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sqlQuery = sqlQuery.And("EvaluationScore_01 <= ?", filter.EvaluationScore01.Max) | |
sqlQuery = sqlQuery.And("EvaluationScore01 <= ?", filter.EvaluationScore01.Max) |
SQL 쿼리에 대한 String이라 반영이 되지 않은 모양이네요..^^
이하 유사 구문에서도 해당 사항 반영이 필요해 보입니다.
EvaluationScore_0X -> EvaluationScore0X
src/api/rest/server/mcis/control.go
Outdated
@@ -282,7 +282,7 @@ func RestDelAllMcis(c echo.Context) error { | |||
|
|||
type RestPostMcisRecommendResponse struct { | |||
//VmReq []TbVmRecommendReq `json:"vmReq"` | |||
Vm_recommend []mcis.TbVmRecommendInfo `json:"vm_recommend"` | |||
VmRecommend []mcis.TbVmRecommendInfo `json:"vm_recommend"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 스타일로 JSON tag 명칭도 유사하게 변경을 부탁 드립니다.
변수명에서 첫 글자만 소문자로 변경하여, json: xxx 를 업데이트 해주시면 되겠습니다.
VmRecommend []mcis.TbVmRecommendInfo `json:"vm_recommend"` | |
VmRecommend []mcis.TbVmRecommendInfo `json:"vmRecommend"` |
src/core/mcis/control.go
Outdated
@@ -454,7 +454,7 @@ type SshCmdResult struct { // Tumblebug | |||
|
|||
// AgentInstallContentWrapper ... | |||
type AgentInstallContentWrapper struct { | |||
Result_array []AgentInstallContent `json:"result_array"` | |||
ResultArray []AgentInstallContent `json:"result_array"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 스타일로 JSON tag 명칭도 유사하게 변경을 부탁 드립니다.
변수명에서 첫 글자만 소문자로 변경하여, json: xxx 를 업데이트 해주시면 되겠습니다.
ResultArray []AgentInstallContent `json:"result_array"` | |
ResultArray []AgentInstallContent `json:"resultArray"` |
src/core/mcir/spec.go
Outdated
OsType string `json:"os_type"` | ||
NumvCPU uint16 `json:"num_vCPU"` | ||
NumCore uint16 `json:"num_core"` | ||
MemGiB uint16 `json:"mem_GiB"` | ||
StorageGiB uint32 `json:"storage_GiB"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 스타일로 JSON tag 명칭도 유사하게 변경을 부탁 드립니다.
변수명에서 첫 글자만 소문자로 변경하여, json: xxx 를 업데이트 해주시면 되겠습니다.
OsType string `json:"os_type"` | |
NumvCPU uint16 `json:"num_vCPU"` | |
NumCore uint16 `json:"num_core"` | |
MemGiB uint16 `json:"mem_GiB"` | |
StorageGiB uint32 `json:"storage_GiB"` | |
OsType string `json:"osType"` | |
NumvCPU uint16 `json:"numvCPU"` | |
NumCore uint16 `json:"numCore"` | |
MemGiB uint16 `json:"memGiB"` | |
StorageGiB uint32 `json:"storageGiB"` |
src/core/mcir/spec.go
Outdated
CostPerHour float32 `json:"cost_per_hour"` | ||
NumStorage uint8 `json:"num_storage"` | ||
MaxNumStorage uint8 `json:"max_num_storage"` | ||
MaxTotalStorageTiB uint16 `json:"max_total_storage_TiB"` | ||
NetBwGbps uint16 `json:"net_bw_Gbps"` | ||
EbsBwMbps uint32 `json:"ebs_bw_Mbps"` | ||
GpuModel string `json:"gpu_model"` | ||
NumGpu uint8 `json:"num_gpu"` | ||
GpuMemGiB uint16 `json:"gpumem_GiB"` | ||
GpuP2p string `json:"gpu_p2p"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 스타일로 JSON tag 명칭도 유사하게 변경을 부탁 드립니다.
변수명에서 첫 글자만 소문자로 변경하여, json: xxx 를 업데이트 해주시면 되겠습니다.
CostPerHour float32 `json:"cost_per_hour"` | |
NumStorage uint8 `json:"num_storage"` | |
MaxNumStorage uint8 `json:"max_num_storage"` | |
MaxTotalStorageTiB uint16 `json:"max_total_storage_TiB"` | |
NetBwGbps uint16 `json:"net_bw_Gbps"` | |
EbsBwMbps uint32 `json:"ebs_bw_Mbps"` | |
GpuModel string `json:"gpu_model"` | |
NumGpu uint8 `json:"num_gpu"` | |
GpuMemGiB uint16 `json:"gpumem_GiB"` | |
GpuP2p string `json:"gpu_p2p"` | |
CostPerHour float32 `json:"costPerHour"` | |
NumStorage uint8 `json:"numStorage"` | |
MaxNumStorage uint8 `json:"maxNumStorage"` | |
MaxTotalStorageTiB uint16 `json:"maxTotalStorageTiB"` | |
NetBwGbps uint16 `json:"netBwGbps"` | |
EbsBwMbps uint32 `json:"ebsBwMbps"` | |
GpuModel string `json:"gpuModel"` | |
NumGpu uint8 `json:"numGpu"` | |
GpuMemGiB uint16 `json:"gpuMemGiB"` | |
GpuP2p string `json:"gpuP2p"` |
src/core/mcir/spec.go
Outdated
OsType string `json:"os_type"` | ||
NumvCPU Range `json:"num_vCPU"` | ||
NumCore Range `json:"num_core"` | ||
MemGiB Range `json:"mem_GiB"` | ||
StorageGiB Range `json:"storage_GiB"` | ||
Description string `json:"description"` | ||
CostPerHour Range `json:"cost_per_hour"` | ||
NumStorage Range `json:"num_storage"` | ||
MaxNumStorage Range `json:"max_num_storage"` | ||
MaxTotalStorageTiB Range `json:"max_total_storage_TiB"` | ||
NetBwGbps Range `json:"net_bw_Gbps"` | ||
EbsBwMbps Range `json:"ebs_bw_Mbps"` | ||
GpuModel string `json:"gpu_model"` | ||
NumGpu Range `json:"num_gpu"` | ||
GpuMemGiB Range `json:"gpumem_GiB"` | ||
GpuP2p string `json:"gpu_p2p"` | ||
EvaluationStatus string `json:"evaluationStatus"` | ||
EvaluationScore01 Range `json:"evaluationScore_01"` | ||
EvaluationScore02 Range `json:"evaluationScore_02"` | ||
EvaluationScore03 Range `json:"evaluationScore_03"` | ||
EvaluationScore04 Range `json:"evaluationScore_04"` | ||
EvaluationScore05 Range `json:"evaluationScore_05"` | ||
EvaluationScore06 Range `json:"evaluationScore_06"` | ||
EvaluationScore07 Range `json:"evaluationScore_07"` | ||
EvaluationScore08 Range `json:"evaluationScore_08"` | ||
EvaluationScore09 Range `json:"evaluationScore_09"` | ||
EvaluationScore10 Range `json:"evaluationScore_10"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같은 스타일로 JSON tag 명칭도 유사하게 변경을 부탁 드립니다.
변수명에서 첫 글자만 소문자로 변경하여, json: xxx 를 업데이트 해주시면 되겠습니다.
@computerphilosopher
기타 참고:
|
확인했습니다. 주말 중으로 다시 커밋 하겠습니다. 하나의 커밋으로 Rebase 할까요? |
@computerphilosopher 꼭 1개의 컷밋으로 스쿼싱할 필요는 없습니다. 히스토리 관리 차원에서, 구분했을 때 독자적으로 의미를 지니는 커밋들의 경우 나눠져 있어도 좋을 것 같습니다. 해당 PR은 2개의 커밋으로 나뉘면 좋을 것 같습니다.
그러나~ 커밋 스쿼싱이 의무 사항은 아니므로 상황에 따라 처리해주시면 될 것 같습니다..^^ |
다시 커밋하였습니다. cb-tumblebug/src/core/mcir/spec.go Line 1025 in 420d0e6
이 함수에서도 언더바를 빼야 하는지 헷갈리는데 API와 관련된 함수 같아서 우선 고치지 않았습니다. |
ca77b3f
to
9af8e1f
Compare
네 ^^ 변수명뿐만 아니라 동일한 명칭을 가지고 있는 string 도 변경이 필요합니다. 해당 커밋을 기준으로 ~ 직접 내려 받아서 전체적으로 살펴보도록 하겠습니다. 감사합니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher PTAL! 😊
src/core/mcir/vnet.go
Outdated
type SpiderVPCReqInfo struct { // Spider | ||
Name string | ||
IPv4_CIDR string | ||
IPv4CIDR string | ||
SubnetInfoList []SpiderSubnetReqInfo | ||
//SubnetInfoList []SpiderSubnetInfo | ||
} | ||
|
||
type SpiderSubnetReqInfo struct { // Spider | ||
Name string | ||
IPv4_CIDR string | ||
IPv4CIDR string | ||
KeyValueList []common.KeyValue | ||
} | ||
|
||
type SpiderVPCInfo struct { // Spider | ||
IId common.IID // {NameId, SystemId} | ||
IPv4_CIDR string | ||
IPv4CIDR string | ||
SubnetInfoList []SpiderSubnetInfo | ||
KeyValueList []common.KeyValue | ||
} | ||
|
||
type SpiderSubnetInfo struct { // Spider | ||
IId common.IID // {NameId, SystemId} | ||
IPv4_CIDR string | ||
IPv4CIDR string | ||
KeyValueList []common.KeyValue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher
올려 주신 PR을 제 개발 환경에 받고, 테스트 스크립트를 돌려 보았습니다.
그랬더니, 동작 상 이슈가 있네요.. 🤔
Tumblebug이 CSP에 vNet을 생성하기 위하여 Spider의 REST API를 호출합니다.
이 때 (Spider의 REST API 호출 시), 여기에 있는 struct를 이용하여 JSON body를 만드는데,
Spider 쪽은 아직 IPv4_CIDR
필드로 입력받고 있으므로
(Spider 쪽 코드가 변경되기 전 까지는)
Tumblebug 코드도 다시 IPv4_CIDR
로 바꿔야 할 것 같습니다 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@computerphilosopher @jihoon-seo 해당 PR 은 마무리 차원에서 제가 직접 확인 및 수정하고 있습니다.
IPv4_CIDR
는 말씀하신대로 한시적으로 복구해두겠습니다. CB-TB nNet 생성 요구사항에 Spider의 struct를 직접 사용하고 있어서, CB-TB struct 로 재정의 하고 나서 정리하는 것이 좋을 것 같네요.
정리하는 것이 좋을 듯 하네요. (camelCase 의 Spider 까지 전파 여부는 잘 모르겠습니다..^^)
@jihoon-seo @computerphilosopher 3개의 커밋을 추가하여 해당 PR을 보완하였습니다. (Maintainer 권한으로 업데이트) [업데이트 사항]
[테스트 사항]
|
@computerphilosopher 제안하신 내용이 main branch에 머지되었습니다! :0 개인적으로는 해당 사항이 코드를 보며 늘 신경쓰이던 부분이었습니다. (API를 변경하게 되는 사항이라 선뜻 손대지 못했던..) 기여해주신 덕분에 이번에 깔끔해졌네요^^ 감사합니다! |
@all-contributors please add @computerphilosopher for code |
I've put up a pull request to add @computerphilosopher! 🎉 |
#654 관련 PR입니다.
변수 이름에서 underscore를 제거하고 camel case로 바꿨습니다.
Make 되는 것 확인했습니다.