Repository navigation
Conversation
Signed-off-by: Josh Lin <linynjosh@gmail.com>
Signed-off-by: Josh Lin <linynjosh@gmail.com>
Signed-off-by: Josh Lin <linynjosh@gmail.com>
goneall
left a comment
goneall
left a comment
There was a problem hiding this comment.
Thanks @linynjosh for the code updates! This should help especially when using it to analyze a larger dataset.
Just a couple of suggestions - neither are required from my perspective.
I also noticed we have a merge conflict, so you may need to merge your changes into the latest code.
| except: | ||
| if i == len(buildermodules) - 1: | ||
| raise FileTypeError("FileType Not Supported" + str(fn)) | ||
|
|
There was a problem hiding this comment.
This code will work, but I would personally find it more readable if on the exception you just continued and raise the error after the for loop since if you made it there, all file types would fail parsing.
I would also change the message to something like "The file cannot be parsed by the RDF, XML, YAML, JSON or Tag/Value parsers".
| raise FileTypeError("FileType Not Supported" + str(fn)) | ||
| buildermodules = [rdfbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, tagvaluebuilders] | ||
| parsing_modules = [rdf, xmlparser, yamlparser, jsonparser, tagvalue] | ||
| read_datas = [False, False, False, False, True] |
There was a problem hiding this comment.
Not a necessary change from my perspective, but it would be nice to attempt parsing based on the file type as the first attempt just for performance reasons.
You could do something like leaving in the code for setting the parsing module and attempting that parse first and only looping through if the parsing_module is not set or if the parse fails.
There was a problem hiding this comment.
I agree with @goneall here. I would hope that by default the files have correct file types and we could parse them based on that. This as the default first try should therefore improve performance. Clearly, as your example from the DocFest shows, this is not always true, so I understand why this feature can be useful.
Additionally I would generate a list of triplets here and iterate through this list as this would be less error-prone. If for example a new buildder and parser is added, one could easily forget to adapt the read_datas list or the order could change so that builder and parser module don't match.
|
See #223 for the previous version of this PR and a bunch of comments which are still valid. |
There was a problem hiding this comment.
Thanks for your PR @linynjosh!
After some reflection and with regard to @goneall's comment, I see the benefit of this new feature and support this in general. (I will create an issue so that we also implement this feature in the refactored data model).
However, I think we should change the test-setup: Instead of adding various new files to the repo (which in the end are only duplications of the existing ones and don't add any value), I would suggest to implement a fixture that only temporarily changes the ending of the file under test. With that we could use the existing test files in format (see also the third bullet point in this comment).
Additionally, we could make one "positive" test that passes if the files could be parsed with the correct ending, one test that passes if files with wrong ending can be parsed and potentially also one test that passes if the FileTypeError is correctly raised. Overall, we should ensure that FileTypeError is only raised if the content of the file doesn't match any of JSON/ YAML/ XML/ RDF or TV as commented below.
Edit: One additional remark on the merge conflict and the failing pipeline: This definitely needs to be fixed before merging this PR but at least the merge conflicts shouldn't be complicated. As I can't see the Details for the failing pipeline I am not sure about this, but this will probably also be fixed with the requested changes.
| raise FileTypeError("FileType Not Supported" + str(fn)) | ||
| buildermodules = [rdfbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, tagvaluebuilders] | ||
| parsing_modules = [rdf, xmlparser, yamlparser, jsonparser, tagvalue] | ||
| read_datas = [False, False, False, False, True] |
There was a problem hiding this comment.
I agree with @goneall here. I would hope that by default the files have correct file types and we could parse them based on that. This as the default first try should therefore improve performance. Clearly, as your example from the DocFest shows, this is not always true, so I understand why this feature can be useful.
Additionally I would generate a list of triplets here and iterate through this list as this would be less error-prone. If for example a new buildder and parser is added, one could easily forget to adapt the read_datas list or the order could change so that builder and parser module don't match.
| @pytest.mark.parametrize("test_file", test_files) | ||
| def test_parse_anything_formats(test_file): | ||
| doc, error = parse_anything.parse_file(test_file) | ||
|
|
||
| assert not error | ||
|
|
There was a problem hiding this comment.
As these two tests are basically the same, I would suggest to use a parametrized test here. Apart from that, as commented above, I don't think that we need this additional test.
| return p.parse(data) | ||
| else: | ||
| return p.parse(f) | ||
| except: |
There was a problem hiding this comment.
This would basically except everything, I am not sure if it is possible, but I would try to make sure that this only excepts "basic" failure if the parser doesn't match the content of the file. So e.g. if we try to use the rdf parser for a file which has a .json ending but is in tag-value format. This should not except any other Errrors that may occur during parsing, otherwise these errors would simply no longer be visible.
|
The main branch has been replaced with the refactored version of the tools-python. The version this PR refers to can be found in the version/v0.7 branch. Please reopen this PR against that branch while addressing the requested changes from here. |
I tried parsing the SBOMs in the SPDX DocFest 20220127 and more than two thirds of the documents could not be parsed due to their file extensions. This was because most files had the extension .spdx and the parse_anything function assumed that every .spdx file should be parsed as a .tag file. However, there were many .spdx files that were not tag files and an error was thrown before they were parsed. Furthermore, there were examples of files with incorrect file extensions which prevented them from being parsed at all. I discussed with @licquia during the summer and he suggested that I try all the parsers for each file until the correct parser matches the type of the file. Below is piece of code where most .spdx documents fail on.
