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

Drop defusedxml dependency / update package metadata #16

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

sergei-maertens
Copy link
Member

Looks like we were up to date with upstream master, but getting tests to pass locally required changes to pyproject.toml

Only the tostring function was used, which was just an alias to the
lxml.etree.tostring function.
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
It looks like our fork's modifications result in an extra text node
being present in the generated XML, causing the test to crash.

This was probably broken a long time already, but CI wasn't running
so... we never realized it?
This is why all projects should just use black.
This also disables the coverage/coveralls step due to a missing token
and it's not like we're going to act on coverage deficits anyway.
@sergei-maertens sergei-maertens merged commit 07e27a2 into maykin Feb 15, 2024
9 checks passed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What shifted by one and why?

Extra CDATA node because of lxml update?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so from the git history I see that Alexander added some extra key information (passphrase) in the signing flow which isn't there in upstream: a7b306bd#diff-f289c39b13ed7b63ec4515ca0f0a8848757505fd429019558f5b2452487b97d0R799 but I can't really trace down how that results in an extra text node

it could also be the lxml update that results in an extra newline. Looking at the actual XML that is produced, the shift-by-one does seem to make sense. I just dislike how this test is written - why doesn't it use an absolute xpath instead to point to the right node? :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downgraded lxml in my local env, it's due to the lxml 5+ upgrade

@sergei-maertens sergei-maertens deleted the chore/drop-defusedxml-lxml branch April 2, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants