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

element.getchildren() was removed from stdlib #57

Merged
merged 4 commits into from Jan 5, 2022
Merged

element.getchildren() was removed from stdlib #57

merged 4 commits into from Jan 5, 2022

Conversation

jvllmr
Copy link
Contributor

@jvllmr jvllmr commented Jan 3, 2022

@FelixSchwarz
Copy link
Member

Thank you for the patch, looks good to me. Any idea why this did not show up in our test suite? Could you provide a simple test case which fails unless the change is applied?

@jvllmr
Copy link
Contributor Author

jvllmr commented Jan 4, 2022

I added tests. I also added a .gitignore and make the github workflow runnable in PRs to ease contributions to this project. I didn't really know how to assert the return value, so I just asserted to crucial properties.

@jvllmr
Copy link
Contributor Author

jvllmr commented Jan 4, 2022

The test fails with python version 3.9 and up as soon as you use get_children() again.

@jvllmr
Copy link
Contributor Author

jvllmr commented Jan 4, 2022

Oh I just realised that my auto-formatter formatted the .yaml file without my intend. I hope you don't mind. I can change it back otherwise.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Thank you for finding this and adding a fix with a test -- this is great. :)

I requested a few changes to make the test more like the others. No worries about the YAML re-formatting -- it looks better than it did before now.

genshi/tests/input.py Outdated Show resolved Hide resolved
genshi/tests/input.py Outdated Show resolved Hide resolved
genshi/tests/input.py Outdated Show resolved Hide resolved
@jvllmr
Copy link
Contributor Author

jvllmr commented Jan 5, 2022

Thank you for the feedback. I hope I implemented it correctly.

@jvllmr jvllmr requested a review from hodgestar January 5, 2022 13:33
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just waiting for builds to pass.

Thank you again for contributing!

@hodgestar hodgestar merged commit d8a363a into edgewall:master Jan 5, 2022
@jvllmr jvllmr mentioned this pull request Feb 8, 2022
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.

None yet

3 participants