Skip to content

Conversation

jonavellecuerdo
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo commented Oct 13, 2023

What does this PR do?

Update the Python CLI template to include the latest (and greatest 🎉 ) of the DataEng team's standards for Python project code linting, formatting, and security checks. We recently revisited our current practices and made the following key decisions:

Helpful background context

The team has created some Confluence documentation around these decisions:

  • Python Projects Linters
  • Python Projects Pre-Commit
  • .dockerignore file: I essentially copied the contents of the .gitignore file. My assumption was that many of the files we don't want to commit to Github are also files that we don't want to include in a Docker container (e.g. cache files, .env files).

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

  1. Clone this repo.
  2. Check into this branch.
  3. Run make install
  4. Run make lint
  5. Run make test

Includes new or updated dependencies?

YES

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/IN-894

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:
* Simplify and harmonize project configuration and linting methods
with modern and active libraries that augment one another.

How this addresses that need:
* Replace setup.cfg with pyproject.toml
* Use updated pyproject.toml template
* Use updated Makefile template
* Add pre-commit hooks
* Update various modules to address Ruff's lint rules
* Update Pipfile to remove deprecated dev packages
* Update Python version to 3.11

Side effects of this change:
* Updated dependencies and deprecation of setup.cfg

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-894
Copy link
Contributor

@ghukill ghukill 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! Thanks for updating these templates.

I know we still have some open questions about configuring python projects, and maybe there are some extremely low level integration tests we could add (e.g. ping the deployed lambda). But I'd vote to move these agreed upon changes through, and tackle those separately.

Copy link
Contributor

@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.

1 suggestion

@jonavellecuerdo jonavellecuerdo merged commit 40237a5 into main Oct 17, 2023
@jonavellecuerdo jonavellecuerdo deleted the IN-894-update-config-and-linting-defaults branch October 17, 2023 13:13
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