feat: support trimmer compose for trimming multiple IDLs #204
+1,208
−146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DMwangnima
force-pushed
the
feat/trimmer-compose
branch
from
d32ea72
to
be99ee9
Compare
DMwangnima
changed the title
DMwangnima
force-pushed
the
feat/trimmer-compose
branch
from
be99ee9
to
232d28b
Compare
DMwangnima
force-pushed
the
feat/trimmer-compose
branch
from
8fbd6a1
to
093c6a9
Compare
DMwangnima
force-pushed
the
feat/trimmer-compose
branch
from
093c6a9
to
8b944ee
Compare
HeyJavaBean
reviewed
Sep 24, 2024
|
|
||
| func parseAndCheckAST(path string, includeDirs []string, recursive bool) *parser.Thrift { | ||
| ast, err := parser.ParseFile(path, includeDirs, recursive) | ||
| check(err) |
There was a problem hiding this comment.
check 里面是 os.Exit 吗，统一抛出 error，在 main.go 相关的地方再处理 error 然后 os.Exit 吧，不然 sdk 调用的时候容易把进程搞炸
| @@ -77,6 +76,12 @@ func (t *TrimResultInfo) FieldTrimmedPercentage() float64 { | |||
| return float64(t.FieldsTrimmed) / float64(t.FieldsTotal) * 100 | |||
| } | |||
|
|
|||
| type TrimASTWithComposeArg struct { | |||
There was a problem hiding this comment.
为啥不能直接在 TrimASTArgs 上加？要单独包一层
| @@ -19,20 +19,27 @@ import ( | |||
| ) | |||
|
|
|||
| // traverse and remove the unmarked part of ast | |||
| func (t *Trimmer) traversal(ast *parser.Thrift, filename string) { | |||
| func (t *Trimmer) traversal(ast *parser.Thrift) { | |||
There was a problem hiding this comment.
没太懂为啥要封一个doTraversal
另外没看到改动之后 travversal 在哪里被调用了？直接把 t.countStructs 放在 traversal 调用前执行一次就好了吧？
| trimmer, err := newTrimmer(nil, "") | ||
| if err != nil { | ||
| return nil, err | ||
| func TrimASTWithCompose(arg *TrimASTWithComposeArg) (trimResultInfo *TrimResultInfo, err error) { |
| } | ||
| cfg := arg.Cfg | ||
| // When ReadCfgFromLocal is set, local cfg has higher priority and the passed cfg would be ignored | ||
| if arg.ReadCfgFromLocal { |
There was a problem hiding this comment.
读文件这些感觉可以提到上面TrimAST里，doTrimAST里就只做裁切相关的事
| trimmer.trimMethodValid = make([]bool, len(trimMethods)) | ||
| trimmer.forceTrimming = forceTrimming | ||
| trimmer.matchGoName = matchGoName | ||
| for i, method := range trimMethods { |
| t.marks[ast.Filename] = make(map[interface{}]bool) | ||
| t.preProcess(ast, ast.Filename) | ||
| func (t *Trimmer) markAST(ast *parser.Thrift, arg *YamlArguments) { | ||
| t.refresh(ast, arg) |
There was a problem hiding this comment.
arg放到trimmer的 field 里传递可能会好些？
| t.markInclude(ast.Includes[theType.Reference.Index], filename) | ||
| } | ||
|
|
||
| if theType.IsTypedef != nil { | ||
| t.markTypeDef(theType, baseAST, filename) | ||
| t.markTypeDef(theType, baseAST) |
There was a problem hiding this comment.
我看你把所有mark的 filename 都去掉了，改为从ast拿，这里我不能完全确定对不对，需要保证 filename 一定和 ast 一致。但似乎看这一行，和原来的代码行为可能就会有出入了（baseAST可能变成别的，但filename是不变的，到你的实现里，传入的filename就变了）
| return | ||
| } | ||
| t.marks[filename][str] = true | ||
| t.marks[ast.Filename][str.Name] = true |
There was a problem hiding this comment.
原本这之前是用指针，现在改成了用string为key，是有啥必要吗？(虽然直觉看着也是唯一的)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Related Issue