Skip to content

Conversation

@ChrisBarker-NOAA
Copy link

This PR removes the six dependency, and all other Py2 compatibility code I could find.

Still a lot of u"strings", but they do no harm.

Also a few changes to remove deprecation warnings.

And a small change to make pytest a bit happier -- I really like pytest for running tests.

I haven't run coverage, so not completely sure if it's been fully tested, but I did make sure most of teh changes broke a test before I fixed it, so it's certainly mostly there.

I couldn't test without six installed, as it's needed by a dependency, but I did grep for "six", and I'm pretty sure I got it all.

@stankudrow
Copy link
Contributor

stankudrow commented Dec 18, 2024

@ChrisBarker-NOAA , hello.

Could you pass all python files via pyupgrade or smth?

@Kludex
Copy link
Member

Kludex commented Dec 18, 2024

Don't run any automated tools yet, please. It's easier to review manual changes first.

Comment on lines +6 to +7
# named with underscore so as not to confuse pytest
class _TestData(BSONCoding):
Copy link
Contributor

Choose a reason for hiding this comment

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


def test_generation_time(self):
d1 = datetime.datetime.utcnow()
d1 = datetime.datetime.now(datetime.UTC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it work in Py3.9?

Copy link
Author

Choose a reason for hiding this comment

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

Darn -- nope.

Which is really annoying -- why depreciate when supported versions don't support the new way?

Good reason to get the CI going!

I don't have time to fix this right now, but I allowed maintainers to push to my branch, so you can do it -- or wait for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, and a PR for the CI/CD is already opened: #131

Copy link
Author

Choose a reason for hiding this comment

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

OK -- fixed this

@ChrisBarker-NOAA
Copy link
Author

All looks good now -- passing on py 3.9 -- 3.13.

There some more formatting cleanup to be done, but as suggested, I think a separate PR is called for there.

@ChrisBarker-NOAA
Copy link
Author

Once #129 is merged, we'll probably want to dump the changes made here in the workflow.

@stankudrow
Copy link
Contributor

Once #129 is merged, we'll probably want to dump the changes made here in the workflow.

Already merged with other PRs, so when rebased, I'll look through as soon as possible.

@stankudrow
Copy link
Contributor

@ChrisBarker-NOAA , herllo. Are you ready to rebase the PR and we can go on with reviewing?

@ChrisBarker-NOAA ChrisBarker-NOAA deleted the remove_six branch December 21, 2024 00:29
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