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

chore: remove pyrfc3339 and change to datetime.datetime.fromisoformat… #1247

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EdmilsonRodrigues
Copy link
Contributor

…() and datetime.datetime.isoformat()

Description

I tried using backports.datetime_fromisoformat but than the tox tests broke, probably because it required an import in the unit tests. I tried than just using the usual builtin datetime.datetime.fromisoformat and it worked just well.

<Fixes: >

QA Steps

<Commands / tests / steps to run to verify that the change works:>

tox -e py3 -- tests/unit/...
tox -e integration -- tests/integration/...

All CI tests need to pass.

<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>

Notes & Discussion

*This is a secondary branch from the Pull Request #1243 *

@james-garner-canonical
Copy link
Contributor

I tried using backports.datetime_fromisoformat but than the tox tests broke, probably because it required an import in the unit tests

Yeah unfortunately we currently need to specify the dependencies in both pyproject.toml and tox.ini we can add backports-datetime-fromisoformat -- you can replace the pyRFC339 dependency in both

I tried than just using the usual builtin datetime.datetime.fromisoformat and it worked just well

I'm guessing you're using python 3.11+ locally -- datetime.fromisoformat was extended in 3.11 to properly support the format rather than just the subset output by datetime. We need to support python 3.8+, and as you can (now) see from the CI run, unit tests for the cookie handling fail on python > 3.11. Hence the need for the backports dependency

BTW as an external contributor your PRs against this repo don't have our CI run automatically, but I believe that you should be able to open a PR against your own fork's main first and have the tests run there -- this will take care of running linting and running tests against multiple python and juju versions

@james-garner-canonical
Copy link
Contributor

Btw @EdmilsonRodrigues we most likely won't be looking at PRs over the next couple of weeks due to the holiday season but happy to continue after that -- thanks for your contributions so far

@dimaqq
Copy link
Contributor

dimaqq commented Dec 20, 2024

I'm sorry about our CI:

   input: chore: remove pyrfc3339 and change to datetime.datetime.fromisoformat() and datetime.datetime.isoformat()
✖   header must not be longer than 100 characters, current length is 105 [header-max-length]

Please amend the commit message 🙇🏻

@EdmilsonRodrigues
Copy link
Contributor Author

I tried to open a PR to my own main branch but no test was created. I think I need to configure the github actions. I'll learn how to do it and do it for the next time.

Also merry holidays for you all!!!

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