Repository navigation
Conversation
| from spdx.parsers import parse_anything | ||
|
|
||
|
|
||
| dirname = os.path.join(os.path.dirname(__file__), "data", "formats") |
There was a problem hiding this comment.
Instead of replacing the old tests with the new ones, can we do both sets of tests?
There was a problem hiding this comment.
Hi Jeff, I added the formats test set back into test_parse_anything. Please tell me if there any other changes you would like me to make.
Signed-off-by: Josh Lin <linynjosh@gmail.com>
Signed-off-by: Josh Lin <linynjosh@gmail.com>
Signed-off-by: Josh Lin <linynjosh@gmail.com>
|
@linynjosh Does this PR fix a specific issue? If it doesn't, can you add a general description of the purpose of the change to the PR? |
nicoweidner
left a comment
nicoweidner
left a comment
There was a problem hiding this comment.
I only understood the purpose of this change (at least what I now believe is the purpose) once I arrived at the test cases - a general description would have been really helpful!
What I now believe is the purpose of this PR: To allow parsing any file correctly, regardless of the file ending. That is, a json document should be parsed correctly, even if the user erroneously named it "document.rdf".
I am skeptical whether this is actually worth going for. While it technically adds some functionality and I don't see any hard reasons against it (at the moment!), I am in favor of failing early if the user submits a file with wrong ending. That is a user error, after all, and it will keep the tool simpler if we just attempt to parse once with the appropriate parser instead of applying "magic". As the tool grows more complex, there may be other issues arising from auto-detection (for example, format-specific options would become a bit weird to handle).
Finally, if we would auto-detect the input format, the CLI tool parameters would have to be changed as well, since the --from parameter would become redundant. For reference, the java tools also respect the specified input format and don't attempt to parse the input file with any parser available (this is the method it boils down to).
Opinions from other contributors would be great! (@licquia?)
| buildermodules = [rdfbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, jsonyamlxmlbuilders, tagvaluebuilders] | ||
| parsing_modules = [rdf, xmlparser, yamlparser, jsonparser, tagvalue] | ||
| read_datas = [False, False, False, False, True] | ||
| for i in range(len(buildermodules)): | ||
| parsing_module = parsing_modules[i] | ||
| buildermodule = buildermodules[i] | ||
| read_data = read_datas[i] | ||
| try: | ||
| p = parsing_module.Parser(buildermodule.Builder(), StandardLogger()) | ||
| if hasattr(p, "build"): | ||
| p.build() | ||
| with open(fn) as f: | ||
| if read_data: | ||
| data = f.read() | ||
| return p.parse(data) | ||
| else: | ||
| return p.parse(f) | ||
| except: | ||
| if i == len(buildermodules) - 1: | ||
| raise FileTypeError("FileType Not Supported" + str(fn)) |
There was a problem hiding this comment.
If I understand correctly, you are trying to:
- loop over all the available builder/parsing modules
- let each one try to parse
- catch any exceptions, and if the last one throws an exception, decide that it's an unsupported file type
Tbh, I don't like this approach at all. Exceptions should not be used for control flow, and this also creates a lot of unnecessary calls. There are also three arrays buildermodules, parsing_modules and read_datas which are linked in a fairly weak way, so if some developer makes a change to, say, buildermodules in the future, they could easily get out of sync.
I would prefer a different approach that is closer to the previous version, but a bit more structured:
- Create an enum containing all the supported file types
- Create a helper class (maybe
FileTypeModules?) containingparsing_module,builder_moduleandread_datafields. You could use@dataclassto avoid writing default constructors, see e.g. https://stackoverflow.com/questions/48254562/python-equivalent-of-typescript-interface - Create a dictionary with the enum values as key and an appropriate instance of the
FileTypeModulesclass as value (this can be at the top level since it is a static dictionary) - After getting the file ending, just look up the key in the dictionary to get the correct modules. If the key is not found, go to
FileTypeError
What do you think?
| @@ -0,0 +1,223 @@ | |||
| { | |||
There was a problem hiding this comment.
About all these new test files:
- As far as I can tell, they are simply duplicated from the existing test files under
tests/data/formats - The different endings appear to be artificial (presumably to test whether a wrongly labelled file will still be parsed correctly)
Thoughts on this:
- I would like to reduce duplication. For the original (correctly labelled) test files, we could use the existing ones. For the wrongly labelled ones, it would require some "before all tests" setup (creating the additional files with other endings) and "after all tests" cleanup (deleting the created files). From some quick googling, I think something with pytest fixtures should work, but I don't have experience with that myself. I'd also be fine with leaving this as a future improvement
- After some deliberation, I don't think the approach of trying to parse a file with all parsers is worth going for. I will comment on this separately, since it concerns all of this PR
| dirname = os.path.join(os.path.dirname(__file__), "data", "formats") | ||
| test_files = [os.path.join(dirname, fn) for fn in os.listdir(dirname)] |
There was a problem hiding this comment.
I don't like overwriting the existing variables here, can you create new ones for the second test suite?
On that note, though: It seems like the old tests are actually included in the new ones. In that case, there does not seem to be a point in repeating the old cases yet another time.
cc @licquia since you requested the old ones to be re-added
| except: | ||
| if i == len(buildermodules) - 1: | ||
| raise FileTypeError("FileType Not Supported" + str(fn)) |
There was a problem hiding this comment.
One more comment about this except: block: It will hide any exceptions that may be thrown by the parsers. This does not include validation errors (since those do not actually raise exceptions), but if anything goes wrong for other reasons, it won't be visible.
|
@linynjosh, do you plan to address this PR again? |
|
Closing this due to inactivity. Please ping me in case it should be reopened. |
4 participants
No description provided.