Improve CLI output name handling by adding file extensions #2914
Conversation
| if (outputName.isEmpty()) return "default.cc.json" | ||
| val delimiter = extractDelimiter(outputName) | ||
| if (delimiter.isEmpty()) { | ||
| return outputName.substringBefore(".") + ".cc.json" | ||
| } |
There was a problem hiding this comment.
It seems like that the code style has not been formatted automatically.
I recommend to always add curly braces even for if-statements with one line.
There was a problem hiding this comment.
I am using the gradle target ktlintFormat and have enabled the codestyle.xml in my intellij-settings.
I have seen in multiple files that curly braces are not used in if - statements with one line, for instance TokeiImporter.kt and assumed it would be a convention. Do I misunderstand something?
There was a problem hiding this comment.
Everything is fine :) There are a lot of if statements without curly braces from the past. I think we discussed this for the visualization. We agreed to use curly braces for new code there. I think we should also do it in analysis part. But this is just a minor remark.
| if (outputName.isEmpty()) return "default.cc.json" | ||
| val delimiter = extractDelimiter(outputName) | ||
| if (delimiter.isEmpty()) { | ||
| return outputName.substringBefore(".") + ".cc.json" |
There was a problem hiding this comment.
This will not work if multiple dots are provided in the fileName like: "my.output.name". This would lead to my.cc.json which would not be correct. Instead you can use substringBeforeLast.
There was a problem hiding this comment.
@ce-bo We were considering using substringBeforeLast. If you are providing a file name such as "test.cc.json", this would lead to test.cc.cc.json which would also be incorrect. Therefore we decided that by convention fileNames do not involve dots.
There was a problem hiding this comment.
Restricting the filename would not be intuitive. So let's not do this.
In case of "test.cc.json" we can simply check if "cc.json" is included or not. If yes, stop filename processing, if not, continue.
| } | ||
|
|
||
| private fun extractPath(outputName: String, delimiter: String): String { | ||
| return outputName.split(delimiter).dropLast(1).joinToString(delimiter) + delimiter |
There was a problem hiding this comment.
I would prefere existing Utility functions (examples: https://mkyong.com/java/how-to-get-the-filepath-of-a-file-in-java/).
Thus, different operating system depending pathes are handled automatically.
| } | ||
|
|
||
| private fun extractFileName(outputName: String, delimiter: String): String { | ||
| val fileName = outputName.split(delimiter).reversed().get(0) |
There was a problem hiding this comment.
Here I would do the same like in comment
| private fun extractFileName(outputName: String, delimiter: String): String { | ||
| val fileName = outputName.split(delimiter).reversed().get(0) | ||
| if (fileName.isEmpty()) { | ||
| return "default.cc.json" |
There was a problem hiding this comment.
Let's create a constant for "default.cc.json" string.
| if (fileName.isEmpty()) { | ||
| return "default.cc.json" | ||
| } | ||
| return fileName.substringBefore(".") + ".cc.json" |
There was a problem hiding this comment.
Use substringBeforeLast instead.
...ysis/model/src/test/kotlin/de/maibornwolff/codecharta/serialization/OutputFileHandlerTest.kt
Show resolved
Hide resolved
| @@ -22,7 +22,7 @@ class ProjectGenerator(private val writer: Writer, private val filePath: String, | |||
| project = MergeFilter.mergePipedWithCurrentProject(pipedProject, project) | |||
| } | |||
|
|
|||
| if (toCompress && filePath != "notSpecified") { | |||
| if (toCompress && filePath != "default.cc.json") { | |||
There was a problem hiding this comment.
Let's add/reuse the constant for default.cc.json
There was a problem hiding this comment.
I could not find any file of constants in the project. Ist there one? Otherwise I would suggest to start one for all the importers (hence, in the directory: import).
There was a problem hiding this comment.
I think YAGNI. Let's start with just one constant in the ProjectGenerator or in the OutputFileHandler.
| BufferedWriter(FileWriter(outputFile!!)) | ||
| } else { | ||
| OutputStreamWriter(output) | ||
| if (!test) { |
There was a problem hiding this comment.
Is this condition only for debugging? What is variable test used for?
There was a problem hiding this comment.
no this is for executing the parser dialog tests. previously the default condition was to write the output in terminal. now a default file is created. during the build it would not be convenient to generate all the default files (also many tests would need to be corrected) therefore we still write the output of .jsons during the build in terminal.
There was a problem hiding this comment.
I think we should not change code only to be able to run unit tests properly. Why has the default behavior changed?
There was a problem hiding this comment.
I can see your point, but I thought the requirement was if the user does not enter any input into the interactive terminal we still create a default cc.json file in the working directory.
There was a problem hiding this comment.
Ah okay. I think this is not included in the scope of this issue. Now it is a bit mixed.
I would rather name the default something like this INPUT-FILE-NAME-output.cc.json. But ideally we should change this in a separate PR. Currently the default output name is handled in de.maibornwolff.codecharta.tools.interactiveparser.ParserDialogInterface#getOutputFileName and simply the input file name is used as the default output filename. But it seems that not all parsers/importers are using this method.
.../JasomeImporter/src/main/kotlin/de/maibornwolff/codecharta/importer/jasome/JasomeImporter.kt
Outdated
Show resolved
Hide resolved
|
@ce-bo I resolved your remarks in my latest commit. |
| /** | ||
| private fun writer(): Writer { | ||
| return outputFile?.writer() ?: System.out.writer() | ||
| } | ||
|
|
||
| **/ |
|
Let‘s Not change the default behavior when the User does not provide a output file name. Thus, the new systemout flag is not needed to be added in every parser. In the Parser Dialog Interface, I think it is called InteractiveParser or similar, we can extend the method for providing the output file, so that the correct file ending is added if necessary. We can suggest a default Output file Name to the user in the interactive usage but the User should still be possible to not set any Output file name, so that the results are going to be dumped to System.out. |
|
@ce-bo Sorry I'm still a bit confused. Do we want to achieve the following two Cases?
So the changes with the sysout flag need to be removed again and move the functionality of the new OutputFileHandler into the ParserDialogInterface? |
|
Yes, that is what I ment. Moving the OutputFileHandler can be checked. I did not have a deep look into that. |
MW-Friedrich
changed the title
Refactor/simplify CrococosmoImporter, so it handles suffixes properly Enhance tests for Crococosmo and MetricGardener #2800
…me-by-adding-file-extensions' into fix/2800/improving-cli-output-name-by-adding-file-extensions
…me-by-adding-file-extensions' into fix/2800/improving-cli-output-name-by-adding-file-extensions
…me-by-adding-file-extensions' into fix/2800/improving-cli-output-name-by-adding-file-extensions
Add more tests for MergeFilter #2800
Refactor all Importers/Parsers for more consistent line breaks #2800
|
[CodeCharta Analysis] Kudos, SonarCloud Quality Gate passed! |
|
[CodeCharta Visualization] Kudos, SonarCloud Quality Gate passed!
|
MW-Friedrich
deleted the
fix/2800/improving-cli-output-name-by-adding-file-extensions
branch
{Meaningful title}
Please read the CONTRIBUTING.md before opening a PR.
Issue: #2800 Improving CLI output name handling by adding file extensions if necessary
Description
Descriptive pull request text, answering:
OutputFileHandler.ktgolden_test.shandTokeiImporterTest.ktproperly the variable systemout has been introduced to enable console outputgolden_test.shneeds to be updated. Importers that are being used in production aren't tested there (e.g. metricgardener, gitlog...). Importers that are not used in production are tested there (eg. crococosmo)OutputFileHandlerwould be transferred to the interfaceParserDialogInterfacebuild.gradle: task integrationtest: It is not possible to run the integrationtests on a windows notebook without having wsl enabled (via bash). All operating systems need to be considered, hence it is always possible to run the integration tests locally.Screenshots or gifs