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

Fix classifier parsing in python 3.8 / python 3.9 #431

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

a1lu
Copy link
Contributor

@a1lu a1lu commented Nov 1, 2022

The classifier parser did not work as expected on my machine.

In python3.8 and python3.9 the type email.message import Message is used as metadata type and the metadata are stored in i_metadata._headers as a list of tuples. To iterate over the classifiers in this list of tuples

for classifier in i_metadata['Classifier']:

was used. Though this loop iterates only over the first classifiers characters and not over all classifiers, so the classifiers were never parsed.

I propose in this commit to generate a list of classifiers for python3.8/python3.9 and python3.10 separately and loop over the list of classifiers.

Please note:
I created this PR on github web, I guess the commit message needs to be adjusted before merging!

@a1lu a1lu requested a review from a team as a code owner November 1, 2022 12:07
@jkowalleck
Copy link
Member

jkowalleck commented Nov 1, 2022

Could you add proper (unit or integration) tests to prove that your changes actually work?
The tests will be running on CI with every supported python version on every operating system.

Additionally, could you please sign your commit according to https://github.com/CycloneDX/cyclonedx-python/pull/431/checks?check_run_id=9224681013 ?

cyclonedx_py/parser/environment.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/environment.py Outdated Show resolved Hide resolved
@jkowalleck
Copy link
Member

jkowalleck commented Nov 1, 2022

thank you for the pull request, @a1lu .

please follow https://github.com/CycloneDX/cyclonedx-python/blob/master/CONTRIBUTING.md

I set your pull-request to "draft" state, as this PR is not ready for review.

@jkowalleck jkowalleck marked this pull request as draft November 1, 2022 12:29
@a1lu
Copy link
Contributor Author

a1lu commented Nov 1, 2022

Could you add proper (unit or integration) tests to prove that your changes actually work? The tests will be running on CI with every supported python version on every operating system.

Sure, Unfortunately I have only access to the github web IDE right now. I can do this probably during this week.

@a1lu
Copy link
Contributor Author

a1lu commented Nov 1, 2022

I added tests to check if both license entries are present. One from the license field and one from the classifiers.

Also I changed the classifier generation, as I discovered that the python3.10 version did not work on poetry run tox. No all versions use the same generator and the tests pass. Though I need to give mypy a hint to not generate an error.

I am going to squash the commits later (I am still only on github web IDE) and sign the commit.

cyclonedx_py/parser/environment.py Outdated Show resolved Hide resolved
tests/test_parser_environment.py Show resolved Hide resolved
cyclonedx_py/parser/environment.py Outdated Show resolved Hide resolved
@jkowalleck
Copy link
Member

jkowalleck commented Nov 2, 2022

Thanks for the code, @a1lu

could you sign your commits according to https://github.com/CycloneDX/cyclonedx-python/pull/431/checks?check_run_id=9242986125
to signal that you are providing your code under the projects licenses?

@jkowalleck
Copy link
Member

@madpah please review this PR :D

The original code did not loop the classifier, but the characters of the
first classifier. This commit fixes this and improves the test case to
test that both, the licence from the License field and the license
classifier is parsed correctly

Signed-off-by: a1lu <github.foreshoe@slmail.me>
@a1lu
Copy link
Contributor Author

a1lu commented Nov 2, 2022

@jkowalleck I squashed my commits and signed them.

Do we also want to parse the license file metadata entry? Or use the get function for "Author" and "License" too? I could try to implement this in new PR.

@jkowalleck
Copy link
Member

jkowalleck commented Nov 3, 2022

@jkowalleck I squashed my commits and signed them.

great 👍


Do we also want to parse the license file metadata entry? Or use the get function for "Author" and "License" too?

I would suggest to open an individual issue for each one of these topics, so we could discuss the need, the details and everything.

@jkowalleck jkowalleck marked this pull request as ready for review November 3, 2022 17:36
@jkowalleck jkowalleck merged commit 4ab075e into CycloneDX:master Nov 10, 2022
@jkowalleck jkowalleck added the bug Something isn't working label Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants