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

Bump python 3.12 and maintenance updates #258

Merged
merged 5 commits into from
Jun 12, 2024
Merged

Bump python 3.12 and maintenance updates #258

merged 5 commits into from
Jun 12, 2024

Conversation

ghukill
Copy link
Contributor

@ghukill ghukill commented Jun 11, 2024

What does this PR do?

Bumps python version to 3.12, and updates structure of project linting and testing to new conventions.

Most changes were syntactical to meet new linting requirements, but a couple of repeated changes are somewhat meaningful:

  • all datetime objects are now UTC timezone aware
    • anywhere datetime.datetime(...) was used, now includes tzinfo=datetime.UTC
    • anywhere datetime.strptime(...) is used, .replace(tzinfo=datetime.UTC) is included to replace the timezone with UTC, but keep the original 00:00 hours/minutes when parsing the YYYY-MM-DD date string without hours or minutes present
      • if tzinfo=... was used, this would bump the hours +4 hours to UTC time upon parsing, which while likely inconsequential, is not ideal, given the YYYY-MM-DD timestamp has no notion of hours or minutes

NOTE: Testing has not yet been confirmed in Dev1 or Stage. This will happen with help from @adamshire123 before a release is tagged.

Helpful background context

This project was using python 3.11 before this PR, and had not receieved any maintenance updates to match new patterns for linting and testing established recently.

How can a reviewer manually see the effects of these changes?

Tests run, linting passes.

As mentioned above, manual integration tests that require access to Terraform and Alma console will be performed with help from @adamshire123 (or someone else with access).

Includes new or updated dependencies?

YES

What are the relevant tickets?

None

Developer

  • All new ENV is documented in README (or there is none)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Why these changes are being introduced:

Some new CLI application project structures, linting, and testing conventions
were established in 2023.  This is the first time this project has receieved
maintenance updates since that time.

Additionally, this commit bumps the python version to 3.12.

How this addresses that need:
* Bumps python version in required areas
* Establishes new linting and testing conventions
* Updates files to meet new linting requirements

Side effects of this change:
* None

Relevant ticket(s):
* None
Copy link

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Looks great! Some minor suggestions

Pipfile Outdated Show resolved Hide resolved
sapinvoices/config.py Outdated Show resolved Hide resolved
tests/test_alma.py Outdated Show resolved Hide resolved
tests/test_sap.py Show resolved Hide resolved
Copy link

@jonavellecuerdo jonavellecuerdo left a comment

Choose a reason for hiding this comment

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

Just a few minor comments but looking good!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
sapinvoices/config.py Outdated Show resolved Hide resolved
sapinvoices/sap.py Outdated Show resolved Hide resolved
@ghukill
Copy link
Contributor Author

ghukill commented Jun 11, 2024

FYI @ehanson8 , @jonavellecuerdo - new commits based on code review suggestions.

@adamshire123
Copy link
Contributor

Tested with sample data in stage environment. LGTM.

@ghukill ghukill merged commit 9a5a488 into main Jun 12, 2024
3 checks passed
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

4 participants