🌱 Extract getConfigVolumes to an interface #372
Conversation
abrugaro
force-pushed
the
extract-providers-to-interface
branch
from
4595a8a to
3450367
Compare
Signed-off-by: Alejandro Brugarolas <[email protected]>
abrugaro
force-pushed
the
extract-providers-to-interface
branch
from
3450367 to
0e885ba
Compare
There was a problem hiding this comment.
LGTM! 👍
@eemcmullan what do you think about the comments regarding refactoring that @abrugaro mentioned? I guess the idea going forward is to split the analyze.go file, which is huge. If we export that struct (and also maybe split it?) we can further refactor this a bit.
| import "github.com/konveyor/analyzer-lsp/provider" | ||
|
|
||
| type Provider interface { | ||
| GetConfigVolume(a *analyzeCommand, tmpDir string) (provider.Config, error) |
There was a problem hiding this comment.
Could probably remove the return err here - I don't notice it being used. We can always add it back later if needed.
There was a problem hiding this comment.
The java provider can throw an error when copying the maven settings file to the temp dir
|
|
||
| import "github.com/konveyor/analyzer-lsp/provider" | ||
|
|
||
| type Provider interface { |
There was a problem hiding this comment.
I think this can be extended to include other methods. In particular, setProviders
| "github.com/konveyor/analyzer-lsp/provider" | ||
| ) | ||
|
|
||
| type DotNetProvider struct { |
There was a problem hiding this comment.
What do you think about moving the provider files into a folder such as providers?
There was a problem hiding this comment.
My initial idea was to place the provider structs in a providers folder. However, the analyzeCommand does not export any parameters, and I didn't want to change that in this PR since I don't have enough context on the code.
Should I change the AnalyzeCommand to export the necessary properties?
There was a problem hiding this comment.
There could be a pkg/providers dir, and export necessary params from the analyze cmd. I think it would be good if it could further be refactored so that the providers were agnostic to the analyze command. This would require a bit more work though, as some of the analyze cmd fields would need to be copied into a map, or similar, to pass to the providers (mostly the Java provider). But, I do think this is a good start to that, so am happy to merge and address these things later.
|
Hi @eemcmullan @jmle , After attempting the refactoring you mentioned in the two comments, I've realized that the changes are going to become quite extensive To extract the providers into another folder, some of the parameters currently in the analyze command would need to be exported. Extracting the providers into another package would create a circular dependency: Could we merge this PR as it is, and I’ll start working on that refactoring in smaller steps to avoid creating a really large PR? |
This PR addresses issue #249. I have modified the
getConfigVolumemethod to extract the configuration creation for each provider into an interface implemented by each corresponding structInitially, my idea was to place the provider structs in an internal package. However, the
analyzeCommanddoes not export any parameters, and I didn't want to change that in this PR since I don't have enough context on the code.I also considered refactoring other elements, such as separating the
analyzeCommanditself from the execution context (which is currently also added toanalyzeCommand). However, I decided not to address this in this PR because I wasn't entirely sure about the scope of the issue@eemcmullan Should I address what I mention in this PR as well? Is there anything else I'm missing regarding the issue?
Thanks!
CC @jmle