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

optimize: generate the go type for const #178

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

luryson
Copy link
Contributor

@luryson luryson commented Apr 1, 2024

Change-Id: I53ea6980c732f71c6fd27b527e24155ce54f8666

Description

add const type

Motivation and Context

const definition is generated without specified type.
It's confusing for client users to find optional values of that type. In my opinion, when typedef types and corresponding values are defined, it is to inform users of the available values of that type.

#174

Related Issue

Change-Id: I53ea6980c732f71c6fd27b527e24155ce54f8666
@luryson luryson requested review from a team as code owners April 1, 2024 07:04
@CLAassistant
Copy link

CLAassistant commented Apr 1, 2024

CLA assistant check
All committers have signed the CLA.

@luryson luryson changed the title WIP: feat: 支持 const 的类型透出 feat: 支持 const 的类型透出 Apr 1, 2024
@HeyJavaBean
Copy link
Member

unit test挂了,可以对应修改下

@HeyJavaBean HeyJavaBean changed the title feat: 支持 const 的类型透出 optimize: generate the go type for const Apr 1, 2024
@luryson
Copy link
Contributor Author

luryson commented Apr 2, 2024

@HeyJavaBean 本地试了多次,好像不是个必现的错误,我先看看能不能浮现出来

@HeyJavaBean
Copy link
Member

看着是其他模块的报错,我来查吧。如果和这个pr没关系的话我就合了

@luryson
Copy link
Contributor Author

luryson commented Apr 2, 2024

调了很多次,捕获到一次这个错误:

=== RUN   TestSimpleGrammar
person_struct_info:failed to parse option:field not exist:name:'123'}, innerStruct:{email
    option_test.go:625: assertion failed
--- FAIL: TestSimpleGrammar (0.01s)

看了一下 idl,有 typedef ,不确定是否有关系

@HeyJavaBean
Copy link
Member

没有关系

@HeyJavaBean HeyJavaBean merged commit dbdeccd into cloudwego:main Apr 2, 2024
3 of 4 checks passed
HeyJavaBean added a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants