Draft: bug fixing #180
Draft: bug fixing #180
Conversation
…sers to support multiple packages per document Signed-off-by: Yan <yyagudayev@vdoo.com>
…ML. Fix parsers to support multiple packages per document. Fixing unit-tests Signed-off-by: Yan <yyagudayev@vdoo.com>
| # Files Analyzed optional | ||
| if pkg_files_analyzed is None: | ||
| return | ||
| if isinstance(pkg_files_analyzed, six.string_types) or isinstance(pkg_files_analyzed, bool): |
There was a problem hiding this comment.
according to the spec, https://spdx.github.io/spdx-spec/3-package-information/
filesAnalysed can only be a boolean.
So I would have stored the conversion to bool.
Also, we are removing six in #179 , so please don't use six for new code/
| def parse_pkg_verif_code_field(self, pkg_verif_code_field): | ||
| """ | ||
| Parse Package verification code dict | ||
| - pkg_verif_code_field: Python dict('value':str/unicode, 'excludedFilesNames':list) | ||
| """ | ||
| if self.document.packages[-1].files_analyzed == False: |
There was a problem hiding this comment.
| if self.document.packages[-1].files_analyzed == False: | |
| if not self.document.packages[-1].files_analyzed: |
There was a problem hiding this comment.
why testing on packages[-1]? Sounds like a hack assuming the current package being parser is the last one.
I think we may rather want to do this test in the caller function
| @@ -1183,6 +1183,7 @@ def p_package_name(self, p): | |||
| value = p[2].decode(encoding="utf-8") | |||
| else: | |||
| value = p[2] | |||
| self.builder.reset_package() | |||
There was a problem hiding this comment.
not sure why this change. Can you please explain?
| package_object["packageVerificationCode"] = self.package_verification_code( | ||
| package | ||
| ) | ||
| if package.files_analyzed in [True, None]: |
There was a problem hiding this comment.
as the default value for this field is True, we should initialize the field to True.
this test is really not obvious, and will lead to bugs in the future
| package_object["licenseConcluded"] = self.license(package.conc_lics) | ||
| package_object["licenseInfoFromFiles"] = list( | ||
| map(self.license, package.licenses_from_files) | ||
| ) | ||
| package_object["licenseDeclared"] = self.license(package.license_declared) | ||
| package_object["copyrightText"] = package.cr_text.__str__() | ||
|
|
||
| if package.has_optional_field("files_analyzed"): |
There was a problem hiding this comment.
as per spec: 3.8.3 Cardinality: Optional, one. If omitted, the default value of true is assumed.
so imho we should really initialize this to True, an only write if it is set to False
| if package.has_optional_field("files_analyzed"): | |
| if package.files_analyzed: |
|
hi @yanyag can you please rebase this PR? now that the six removal PR is merged, there are a few conflicts. |
…rt_spdx_v2.2 apply changes to conform to 2.2 spdx format
apply fixes to adapt schema 2.2
|
Please remember you still have this PR open that needs rebasing. It still contains merge conflicts. Now the PR contains more fixes than initially pushed, and it becoming less trivial. |
|
Gentle ping :) |
|
hi, we will probably need some time until we'll be able to have this MR ready due to capacity. will ping when ready. |
|
gentle ping :) Any update? |
|
As this PR is still in draft mode and has not been receiving vital updates for over a year, I suggest closing it. @yanyag, any objections? |
|
Closing this due to inactivity, please ping if it should be reopened. |
Support writing and parsing "filesAnalyzed" into XML/JSON/YAML.
Fix parsers to support multiple packages per document.
All the tests are passing.