-
Notifications
You must be signed in to change notification settings - Fork 81
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
Pipeline #143
Pipeline #143
Conversation
# Conflicts: # src/main/kotlin/astminer/cli/LabelExtractors.kt
# Conflicts: # src/main/kotlin/astminer/cli/LabelExtractors.kt # src/main/kotlin/astminer/common/model/HandlerModel.kt # src/main/kotlin/astminer/examples/Code2VecJavaMethods.kt
# Conflicts: # build.gradle.kts # src/main/kotlin/astminer/parse/antlr/javascript/JavaScriptMethodSplitter.kt # src/test/kotlin/astminer/cli/Code2VecExtractorTest.kt # src/test/kotlin/astminer/cli/PathContextsExtractorTest.kt # src/test/kotlin/astminer/parse/cpp/FuzzyMethodSplitterTest.kt # src/test/kotlin/astminer/parse/gumtree/python/GumTreePythonMethodSplitterTest.kt
# Conflicts: # src/main/kotlin/astminer/cli/Code2VecExtractor.kt # src/main/kotlin/astminer/cli/LabelExtractors.kt # src/main/kotlin/astminer/cli/ProjectParser.kt # src/main/kotlin/astminer/cli/utils.kt # src/main/kotlin/astminer/common/model/HandlerModel.kt # src/main/kotlin/astminer/examples/AllJavaFiles.kt # src/main/kotlin/astminer/examples/Code2VecJavaMethods.kt # src/main/kotlin/astminer/parse/antlr/javascript/JavaScriptMethodSplitter.kt # src/test/kotlin/astminer/cli/LabelExtractorTest.kt
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.
Some questions:
- Why do you have an empty file (
antlr/javascript/JavaScriptMethodSplitter
) - There are a lot of warnings from kotlin compiler
private fun createStorage(extension: FileExtension): Storage = with(config.storage) { | ||
val storagePath = createStorageDirectory(extension).path | ||
|
||
// TODO: should be removed this later and be implemented like filters and problems, once storage constructors have no side effects |
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.
What side effects do you mention here?
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.
For example the constructor of CsvAstStorage
creates folders on the disk. In this case it is critical because I don't want storageConfig.getStorage()
to create directories
src/test/kotlin/astminer/parse/gumtree/python/GumTreePythonMethodSplitterTest.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/kotlin/astminer/cli/FilterPredicates.kt # src/main/kotlin/astminer/cli/LabelExtractors.kt # src/main/kotlin/astminer/cli/ProjectParser.kt # src/main/kotlin/astminer/common/model/ParsingModel.kt # src/test/kotlin/astminer/common/TestUtils.kt
# Conflicts: # src/main/kotlin/astminer/cli/Code2VecExtractor.kt # src/main/kotlin/astminer/cli/FilterPredicates.kt # src/main/kotlin/astminer/cli/LabelExtractors.kt # src/main/kotlin/astminer/cli/PathContextsExtractor.kt # src/main/kotlin/astminer/cli/ProjectParser.kt # src/main/kotlin/astminer/common/model/ParsingModel.kt # src/main/kotlin/astminer/examples/AllJavaScriptFiles.kt # src/main/kotlin/astminer/examples/AllPythonFiles.kt # src/test/kotlin/astminer/cli/LabelExtractorTest.kt
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.
- Please rename config extensions. YML corresponds to "Yandex Market Language" and in configs, we use "YAML Ain't Markup Language" or simply YAML.
- Also, I don't get how you name those configs. I expect more understandable names, like
function-name-prediction-path-representation.yaml
orfile-asts-csv-storage.yaml
- Create a
branch
package inside thepipeline
package. And move both branches to separate files in this package. I guess storing them all in one file leads to less readability.
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.
It looks very good, still some comments about building branches
Completely replaces old CLI with new pipeline and config