Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Add isort and a corresponding CI check #533

Merged
merged 8 commits into from
May 19, 2022
Merged

Add isort and a corresponding CI check #533

merged 8 commits into from
May 19, 2022

Conversation

PSalant726
Copy link
Contributor

Purpose

Import ordering is unenforced, and frequently messy. This leads to large and often unrelated diffs for engineers who automatically "organize imports on save" in their IDE.

Changes

  • Added isort as a dev-requirement
  • Added isort as a CI check
  • Executed isort to automatically organize all imports: 
    isort src/ tests/

Checklist

  • Update CHANGELOG.md file in appropriate sections
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #532

@PSalant726 PSalant726 added enhancement New feature or request maintenance Refactoring or ongoing maintenance work dev experience Enhancements to the overall DX run unsafe ci checks Triggers running of unsafe CI checks dependencies Pull requests that update a dependency file Tech Debt labels May 18, 2022
@PSalant726 PSalant726 self-assigned this May 18, 2022
@PSalant726 PSalant726 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels May 18, 2022
sanders41
sanders41 suggested changes May 19, 2022
View reviewed changes
Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

This is a good idea to add. It will make code reviews less noisy.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@PSalant726
Differentiate make isort and make isort-ci
dc33a5b 
The `make isort` target will also fix errors, and is intended to be
used locally. The `make isort-ci` target will only alert on errors.
sanders41
sanders41 approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm

@PSalant726 PSalant726 merged commit d4ebae8 into main May 19, 2022
@PSalant726 PSalant726 deleted the add-isort branch May 19, 2022 18:42
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
@PSalant726
Add isort and a corresponding CI check (#533)
454a96f 
* Sort `dev-requirements` alphabetically

* Add `isort`, configuration, `make` target

* Execute `isort` to sort unsorted imports

* Update `CHANGELOG.md`

* Differentiate `make isort` and `make isort-ci`

The `make isort` target will also fix errors, and is intended to be
used locally. The `make isort-ci` target will only alert on errors.

* Sort unsorted imports
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sanders41 sanders41 sanders41 approved these changes

Assignees

@PSalant726 PSalant726

Labels
dependencies Pull requests that update a dependency file dev experience Enhancements to the overall DX enhancement New feature or request maintenance Refactoring or ongoing maintenance work run unsafe ci checks Triggers running of unsafe CI checks Tech Debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add isort as a CI check
2 participants
@PSalant726 @sanders41