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

[MAINTENANCE] use invoke for common contrib/dev tasks #5506

Merged
merged 23 commits into from
Jul 19, 2022

Conversation

Kilo59
Copy link
Member

@Kilo59 Kilo59 commented Jul 14, 2022

Changes proposed in this pull request:

Combing a few developer/contributor quality of life improvements in this PR, but we may want to break these up into separate PRs before merging.

I've moved anything not related to the invoke task file to other PR will be opening those shortly.

  1. Creating a pyproject.toml file to define the build system ge uses and start centralizing tool config to it (black, isort, pytest, etc.)] moved -> [MAINTENANCE] move tool config to pyproject.toml - fork #5525

  2. Begin to use the invoke python command line task runner for running common developer tasks. Define a set of starter tasks like linting, formatting, type checking, etc. These can serve as a guide for future tasks. By providing easy to use and understand generic command line tasks like lint, fmt etc. we make it easy for new contributors to get up and running quickly without having to know anything about the details of our particular tooling. This also makes it easier to change the underlying tools and/or configuration without breaking anyones workflow.
    But contributors are still free to use each of these tools directly.
    Alternatives to invoke: Make, duty, Pipfile scripts.

  3. Building on the excellent work of @cdkini begin to fully type-check select ge modules. moved -> [MAINTENANCE] begin static type-checking #5540

  4. Enforce our pre-commit hooks in the CI (WIP) - use pre-commit ci instead of azure?
    moved -> [MAINTENANCE] enforce pre-commit in ci #5526

image

TLDR Summary

  • add pyproject.toml file and move tool config to it
    • move isort config
    • move pytest config
  • add invoke task runner and define common developer tasks
    • invoke fmt
    • invoke sort
    • invoke lint
    • invoke hooks (run pre-commit hooks)
    • invoke types (type-check?)
    • update CI to use new invoke tasks
  • add check-json pre-commit hook
  • update CI to enforce pre-commit hooks
  • begin "real" mypy static type checking of select modules
    • fix all typing errors on select modules
  • update contrib docs to reflect changes

Definition of Done

Please delete options that are not relevant.

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added unit tests where applicable and made sure that new and existing tests are passing.
  • I have run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented Jul 14, 2022

👷 Deploy request for niobium-lead-7998 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit d1969bc

@Kilo59 Kilo59 self-assigned this Jul 14, 2022
@Kilo59
Copy link
Member Author

Kilo59 commented Jul 14, 2022

@cla-bot check

pyproject.toml Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@billdirks billdirks left a comment

Choose a reason for hiding this comment

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

This looks nice! I have some questions inline. Also, since you are a core developer you can clone and branch in the main repo instead of forking it.

tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Comment on lines 25 to 35
Linting with `isort` MUST occur from a virtual environment that has all required packages installed, and pre-commit uses the virtual environment from which it was installed, whether or not that environment is active when making the commit.

This means you must ensure you have activated a virtual environment that has all development requirements installed **before running `pre-commit install`**.

```console
pre-commit uninstall
# ACTIVATE ENV, e.g.: conda activate pre_commit_env OR source pre_commit_env/bin/activate
pip install -r requirements-dev.txt
pre-commit install --install-hooks
```

Copy link
Member Author

@Kilo59 Kilo59 Jul 18, 2022

Choose a reason for hiding this comment

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

These steps are no longer necessary as of isort 5 and is the reason tools like seed-isort were deprecated.
https://github.com/asottile-archive/seed-isort-config#deprecated

https://pycqa.github.io/isort/docs/configuration/pre-commit.html#seed-isort-config

@Kilo59 Kilo59 changed the title [MAINTENANCE] invoke & contributor QOL updates [MAINTENANCE] use invoke for common contrib/dev tasks Jul 18, 2022
@Kilo59 Kilo59 marked this pull request as ready for review July 18, 2022 19:54
@Kilo59 Kilo59 requested a review from cdkini July 18, 2022 19:54
docs/contributing/style_guides/code_style.md Outdated Show resolved Hide resolved
tasks.py Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
@Kilo59
Copy link
Member Author

Kilo59 commented Jul 19, 2022

@AFineDayFor

little nit and stretch thing, leveraging click to manage args to invoke?

As discussed, the intended use-cases for click and invoke are a bit different.
Invoke is meant to be more of a light-weight, local, Make-like task runner, while click is more of heavier CLI application framework.

Although invoke can be used in the same way and click CAN be used to run common dev tasks.

Copy link
Contributor

@Shinnnyshinshin Shinnnyshinshin left a comment

Choose a reason for hiding this comment

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

Love it... thank you very much for introducing these changes @Kilo59

@Kilo59 Kilo59 enabled auto-merge (squash) July 19, 2022 17:54
@Kilo59 Kilo59 merged commit f088428 into great-expectations:develop Jul 19, 2022
@Kilo59 Kilo59 deleted the maintenance/invoke branch July 21, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants