Skip to content
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

Unable to correctly import CSV from Source Monitor #1886

Closed
silasg opened this issue Mar 19, 2021 · 6 comments · Fixed by #3026
Closed

Unable to correctly import CSV from Source Monitor #1886

silasg opened this issue Mar 19, 2021 · 6 comments · Fixed by #3026
Assignees
Labels
difficulty:low Solving this is likely not that difficult feature Feature requests good first issue java Pull requests that update Java code. Usually created by Dependabot. priority:high Set by PO

Comments

@silasg
Copy link
Member

silasg commented Mar 19, 2021

Bug

GIVEN
Source Monitor CSV exported using current source monitor version for a Delphi code base containing data about one checkpoint with n files (n >> 1)

WHEN

ccsh csvimport my.csv -o mymap.cc.json

THEN Current
Created Project with 1 leaves.

THEN Expected
Created Project with n leaves.

Research shows that the CSV header did not contain "Path" like expected by import filter but "File Name". Changing the header manually fixes the issue. Proposal: check if the header is related to Source Monitor version and change the hard-coded expectation or (better) add a command line switch to overwrite which column to use for node name.

  • CodeCharta Version: 1.68
  • OS: MacOs
  • Browser: -
@silasg silasg added the bug Only issues that describe bugs. label Mar 19, 2021
@ollio
Copy link

ollio commented Apr 12, 2021

@silasg do you can attach the example csv where the error occurs?

@silasg
Copy link
Member Author

silasg commented May 26, 2021

@ollio it was confidential customer data ;/

@BridgeAR BridgeAR added feature Feature requests priority:high Set by PO java Pull requests that update Java code. Usually created by Dependabot. and removed bug Only issues that describe bugs. labels Nov 15, 2021
@BridgeAR BridgeAR added difficulty:low Solving this is likely not that difficult good first issue labels Dec 6, 2021
@BridgeAR BridgeAR added this to the Good first issues milestone Dec 16, 2021
@ce-bo
Copy link
Collaborator

ce-bo commented Aug 8, 2022

Let us ask the user for the name of the file name/path column, so that the user is flexible to specify the right column.

@Hall-Ma
Copy link
Contributor

Hall-Ma commented Aug 31, 2022

@ce-bo
Jan and I did some research about this issue. First of all, the user should use our sourcemonitor importer, when he has a CSV file made by SourceMonitor. Because this importer expects 'File Name' as a part of the header and it works as expected.
Additionally, we have the csv importer which expects 'Path' as a part of the header, and because of this, it can't parse a CSV file from SourceMonitor correctly at the moment.
Interestingly, the csv importer and sourcemonitor importer use the same single CSV file as test ressource, but we have only 'Path' as the header.
Why do we have two (not really) different CSV importers for the same thing? We could save resources if we would maintain only one importer and handle the naming of the header in the code differently (with or without asking the user).
In addition, the user can no longer select the wrong importer.
What are your thoughts?

@ce-bo
Copy link
Collaborator

ce-bo commented Sep 6, 2022

  • The SourceMonitorImporter has custom logic (e.g. to rename column names and to produce a special output csv file). The import is not done by the SourceMonitorImporter itself, it just uses CSVImporter for that. For now I would not invest time to merge the two importers. If we would have only one CSVImporter we still would need a kind of an adapter to implement the column renaming logic.
  • Let's add a new option to the CSVImporter to specify the path column name, so that the user is more flexible during the import.
  • I assume that the path column name for SourceMonitor CSV files keeps stable. If so, we do not need to add the option to the SourceMonitorImporter.
  • As you mentioned, please improve the unit test

@jannikr
Copy link
Contributor

jannikr commented Sep 6, 2022

Thank you for your quick replay @ce-bo. We should not forget to adapt or improve the documentation after the changes so that misunderstandings do not occur again on this topic.

Hall-Ma added a commit that referenced this issue Sep 6, 2022
Hall-Ma added a commit that referenced this issue Sep 6, 2022
jannikr added a commit that referenced this issue Sep 6, 2022
jannikr added a commit that referenced this issue Sep 6, 2022
jannikr added a commit that referenced this issue Sep 6, 2022
jannikr added a commit that referenced this issue Sep 6, 2022
jannikr added a commit that referenced this issue Sep 7, 2022
jannikr added a commit that referenced this issue Sep 7, 2022
* Add option to csv importer to specify the path column name
#1886

* Add unit test
#1886

* Remove comment and adjust file names
#1886

* Update csv parser dialog #1886

* Add unit test to csv dialog #1886

* Rewrite CSVBuilderTest and change to junit
#1886

* Update CHANGELOG.md #1886

* Reformat code #1886

* Update documentation #1886

Co-authored-by: jannikolai.rueckert <jan.nr@live.de>
Co-authored-by: Jan N. Rückert <31436472+jannikr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:low Solving this is likely not that difficult feature Feature requests good first issue java Pull requests that update Java code. Usually created by Dependabot. priority:high Set by PO
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants