refactor(#2): simplify aibolit tool implementation and refactor critic implementation #42
Conversation
21b9a2f to
1d39622
Compare
1d39622 to
51737bc
Compare
|
@h1alexbel Could you review these changes, please? |
h1alexbel
left a comment
There was a problem hiding this comment.
@volodya-lombrozo please check my comments
cmd/root.go
Outdated
| // Currently, we create Aibolit, but it would be great to have such option. Examples of such tools | ||
| // are: `aibolit`, `none`, etc. We should be able to pass multiple tools, for instance: | ||
| // `--tools=aibolit,qulice`. | ||
| // Currently, we create Aibolit, but it would be great to have such option. Examples of such tools |
There was a problem hiding this comment.
@h1alexbel It's auto-formatting (golangci-lint fmt), but I agree, that it is out of the scope here.
internal/brain/metric_brain.go
Outdated
| // print it. Since there is no `PrintStats` method in original `Brain`, it looks ugly when we call this | ||
| // function on `Brain` instance in `refrax_client.go`. We should organize more proper abstraction | ||
| // around the aggregation and printing of stats. | ||
| // Currently, we needed it in order to aggregate all the messages in the `stats` map, and then |
| ) | ||
| aibolit := NewAibolit("Foo.java") | ||
| aibolit.executor = &mockRunner{output: strings.Join(lines, "\n")} | ||
|
|
There was a problem hiding this comment.
@volodya-lombrozo can we remove empty lines in the method body. Check An Empty Line is a Code Smell
There was a problem hiding this comment.
@h1alexbel I would agree with you if it was production code (no tests), but in tests it is an AAA pattern. Check this out: https://stackoverflow.com/a/24070436/10423604
It helps to identify method that is under the test
| } | ||
|
|
||
| func (c* CombinedTool) Imperfections() string { | ||
| func (c *CombinedTool) Imperfections() string { |
There was a problem hiding this comment.
@volodya-lombrozo how about we introduce new GitHub Action, called fmt, that will fail the build, if the sources are not properly formatted, according to the gofmt. WDYT?
internal/critic/server.go
Outdated
| } | ||
| c.log.Info("received messsage #%s, '%s', number of attached files: %d", m.MessageID, task, 1) | ||
| c.log.Info("asking ai to find flaws in the code...") | ||
| var imperfections string |
There was a problem hiding this comment.
@volodya-lombrozo how about we implement a decorator for this logic?
There was a problem hiding this comment.
@h1alexbel I've reused CombinedTool here. What do you think?
|
@h1alexbel Could you have a look one more time, please? |
h1alexbel
left a comment
There was a problem hiding this comment.
@volodya-lombrozo just one comment and we are good to merge
internal/critic/server.go
Outdated
| } | ||
| c.log.Info("received messsage #%s, '%s', number of attached files: %d", m.MessageID, task, 1) | ||
| c.log.Info("asking ai to find flaws in the code...") | ||
| imperfections := NewCombinedTool(c.tool...).Imperfections() |
|
@h1alexbel Thank you for the comprehensive code review |
In this PR I just refactor recent changes. I combine Aibolit with SanitizedAibolit and slightly simplify
criticusage.Related to #2