Python style enforced with pre-commit hook by aedwardg · Pull Request #185 · codeforpdx/dwellinglybackend

aedwardg · 2020-12-30T10:04:56Z

What issue is this solving?

Closes codeforpdx/dwellingly-app/issues/348

This PR adds Flake8-bugbear, Black and Pre-commit dependencies in order to enforce clean and uniform python style throughout the backend.

Once installed, the pre-commit hook will automatically perform the following actions when you try to make a commit:

Add a newline to the end of the file if it doesn't have one
Strip trailing whitespace on every line
Format all Python code in Black style
Lint with Flake8 and Flake8-bugbear for non-formatting errors

If the hook re-formatted any of your code or if you had to fix something to resolve a Flake8 error, you will need to add and commit those files again in git because they have been further modified.

A few files had some specific code changed in order to comply with the new, stricter linting, but most of the changes are formatting.

I tried to keep all important changes in their own commits, with the most recent commit reserved for the non-critical formatting changes.

Any helpful knowledge/context for the reviewer?

Is a re-seeding of the database necessary? I don't think so.
Any new dependencies to install? YES
Any special requirements to test? YES

New instructions for dependency install
Because Black still hasn't released an official version 1.0.0, you have to allow for pre-release packages when installing with pipenv:
pipenv install -d --pre

Also, once you install the dependencies, you will need to install the pre-commit hook for it to work (only once, though we might need to do this again if the pre-commit config gets changed in the future):
pipenv run pre-commit install

Please make sure you've attempted to meet the following coding standards

Code builds successfully
Code has been tested and does not produce errors
Code is readable and formatted
There isn't any unnecessary commented-out code

Added flake8-bugbear, pre-commit and black. Contributing.py will have to be updated to reflect new command to install the deps: `pipenv install -d --pre`

This is so that team members can use the same Flake8 configuration with their editor, before trying to commit. Setting up their editor to lint with Flake8 should save on some headaches when trying to commit.

This is the file that sets up the pre-commit formatting and linting. To install, team members will have to run `pre-commit install` once.

Some of the __init__.py files are used for imports. Flake8 raises an unused import error for these and also complains if the import was a * import. The unused import error can be resolved by putting the name of the class or function being imported into __all__, but there will still be an error if a * import is used. Since we are still actively creating fixtures and other tests that might be imported through these __init__ files, I think we should leave the files be for now and just have Flake8 skip over them during the pre-commit check.

I had to refactor these functions because we were improperly assigning default values to function calls and mutable data structs. Below are the excellent flake8-bugbear warnings that I had to resolve: (for tenant and property fixtures) B006 Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them. (for lease, contact_number, ticket and notes fixtures) B008 Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.

In these tests: - `test_pm_is_authorized_to_get_all` - `test_staff_are_authorized_to_get_all` - `test_admin_is_authorized_to_get_all` creat_lease() was assigned to the variable `lease`, but the variable was never used. I couldn't tell if this function call was needed (the tests still passed when the line was removed), but the variable name definitely wasn't. I just changed the line to a bare function call instead of a variable assignment in order to avoid an unused variable warning.

Added instructions for initial installation of dependencies and the pre-commit hook. Added instructions for the use of pre-commit hook. Still need to add instructions for setting up/using Flake8 and Black on editors/command line

(aka cleaning house) Very little actual code was changed. The majority of changes in this commit were: - reformatting - removing unused imports - removing unused variables - occasionally changing `if not "string" in foo` to `if "string" not in foo` - occasionally changing == to is or != to is not

aedwardg · 2020-12-30T10:22:52Z

You might notice that in conftest.py and app.py there are some comments that say: noqa: followed by some error codes.
This is how to make Flake8 ignore a particular error on that line. If we figure out a way to resolve those two lines, we can remove the comments.

Also, the Flake8 pre-commit is currently configured to ignore __init__ files, since we are importing modules that are not being used directly in __init__ and also using star imports to import all factory fixtures. If we need to ignore additional files in the future (such as auto-generated Alembic files), we'll need to update the pre-commit-config.yaml with the appropriate regex patterns.

aedwardg · 2020-12-30T10:27:21Z

Also, documentation should probably be made on how to set up Flake8 and Black for use on editors and the command line (e.g., format on save and auto-linting). When used properly in your workflow, you won't have to change much at commit time because most of it will already pass the pre-commit checks.

This could maybe be a separate issue?

NickSchimek

Thanks for doing all of this work! I have a few suggestions, but nothing that would stop this from being merged.

NickSchimek · 2020-12-30T10:43:40Z


    def test_pm_is_authorized_to_get_all(self, pm_header, create_lease):
-        lease = create_lease()
+        create_lease()


yeah, we don't need this line. These are testing auths, and it shouldn't matter if there is data or not.

Cool, I thought so, but wasn't sure. I'll delete that line from the three tests.

NickSchimek · 2020-12-30T11:53:26Z

+    emergency_contact = create_emergency_contact()
+
+    def _create_contact_number(emergency_contact=emergency_contact):


this keeps the current behavior intact. However, since these are fixtures, it would be better if each call to the fixture is completely dynamic.
Perhaps we can do something like this instead: It is ugly, but I believe this is the only way to do, what I intended to do, in Python.

def _create_contact_number(emergency_contact=None): if not emergency_contact: emergency_contact = create_emergency_contact()

Okay, I'll do that for the four fixtures that were changed like this one.

NickSchimek · 2020-12-30T11:56:28Z

    - Note: It is not necessary to install python before installing pipenv.
    - Please install pipenv according to their docs for your OS.
-3. Install dependencies `pipenv install -d`
+3. Install dependencies `pipenv install -d --pre`


This is getting to be annoying. I wish pipenv would install everything by default

I agree. One thing we could do is add a new custom script in the [scripts] section of the pipfile.

For example, we could add this line:
get_deps = "pipenv install -d --pre"

Then when we want to install and update dependencies, we would run:
pipenv run get_deps

That would be great! Perhaps naming it pipenv run dev-install

NickSchimek · 2020-12-30T11:59:26Z

+    - The pre-commit hook will check your code every time you try to commit. If your code needs re-formatting, that will happen automatically. If you have non-formatting errors in your code, these will be printed to the terminal with the error, filename and line number. You will need to resolve these yourself.
+    - After your code has been reformatted and/or you have resolved other errors, you will need to add and commit those files again (because they have been modified).
+    - Successful commits can then be pushed to your remote branch like normal.
+


Looks good! Thank you for updating the docs!

NickSchimek · 2020-12-30T12:26:29Z

+        # if this tenant has a lease
+        if (
+            "dateTimeEnd" in leaseData
+            and "dateTimeStart" in leaseData
+            and "propertyID" in leaseData
+        ):


We should probably create an issue to refactor this or do it now. I'd like to remove this completely, but that might not be possible without changing the frontend.

There are two issues here that make it hard to read.

The comment

the multi-line if statement

I would suggest changing it to something like this instead:

if _lease(): ... ... def _lease(): return ( "dateTimeEnd" in leaseData and "dateTimeStart" in leaseData and "propertyID" in leaseData )

That makes sense. I've refactored it and got it working. If it still bothers us down the line, we can always refactor again 😆

NickSchimek · 2020-12-30T12:31:48Z

@@ -1 +1 @@
-python-3.8.2
+python-3.8.2


I don't see any difference. What changed here? 🤔

It added a newline. I'm not sure why GitHub makes it difficult to see whitespace changes sometimes.

NickSchimek · 2020-12-30T12:51:44Z

Also, documentation should probably be made on how to set up Flake8 and Black for use on editors and the command line (e.g., format on save and auto-linting). When used properly in your workflow, you won't have to change much at commit time because most of it will already pass the pre-commit checks.

This could maybe be a separate issue?

Yes, separate issue is fine. This would be great information to have, but I imagine this info is editor dependent and we can't support docs for every editor. I think it would be best if we could redirect to some external documentation.

@NickSchimek

As per @NickSchimek's request, removed unneeded calls to `create_lease()` in `integration/test_leases` and refactored the following factory fixtures: - contact_number - lease - ticket - notes

Now you can run `pipenv run dev-install` instead of `pipenv install -d --pre`

aedwardg · 2020-12-30T15:46:58Z

@NickSchimek thanks for taking the time to review this massive PR!

I made the changes you suggested. Let me know if you see anything else that needs cleaning up.

NickSchimek

This looks great! Thank you!

aedwardg added 8 commits December 29, 2020 20:38

Add dependencies for formatting/styling

a98cfe9

Added flake8-bugbear, pre-commit and black. Contributing.py will have to be updated to reflect new command to install the deps: `pipenv install -d --pre`

Add Flake8 config file

e465ae2

This is so that team members can use the same Flake8 configuration with their editor, before trying to commit. Setting up their editor to lint with Flake8 should save on some headaches when trying to commit.

Add pre-commit config file

446531f

This is the file that sets up the pre-commit formatting and linting. To install, team members will have to run `pre-commit install` once.

Add pre-commit instructions to Contributing.md

76d09e8

Added instructions for initial installation of dependencies and the pre-commit hook. Added instructions for the use of pre-commit hook. Still need to add instructions for setting up/using Flake8 and Black on editors/command line

aedwardg requested a review from NickSchimek December 30, 2020 10:04

aedwardg self-assigned this Dec 30, 2020

NickSchimek previously approved these changes Dec 30, 2020

View reviewed changes

NickSchimek requested a review from a team December 30, 2020 12:43

aedwardg added 4 commits December 30, 2020 07:22

Clean up fixtures and test_leases

a7ffbaa

As per @NickSchimek's request, removed unneeded calls to `create_lease()` in `integration/test_leases` and refactored the following factory fixtures: - contact_number - lease - ticket - notes

Add custom pipenv script for installing dev deps

92f1837

Now you can run `pipenv run dev-install` instead of `pipenv install -d --pre`

Update Contributing.md with new pipenv command

0d85a7a

Update Tenants post resource for readability

e41af54

aedwardg dismissed NickSchimek’s stale review via e41af54 December 30, 2020 15:44

aedwardg requested a review from NickSchimek December 30, 2020 15:45

NickSchimek approved these changes Dec 31, 2020

View reviewed changes

NickSchimek merged commit 82a8f7c into development Dec 31, 2020

NickSchimek deleted the python-style branch December 31, 2020 23:58

aedwardg mentioned this pull request Jan 2, 2021

Advanced setup info/documentation #188

Merged

4 tasks

		emergency_contact = create_emergency_contact()

		def _create_contact_number(emergency_contact=emergency_contact):

		@@ -1 +1 @@
		python-3.8.2 No newline at end of file
		python-3.8.2

Conversation

aedwardg commented Dec 30, 2020

What issue is this solving?

Any helpful knowledge/context for the reviewer?

Please make sure you've attempted to meet the following coding standards

Uh oh!

aedwardg commented Dec 30, 2020

Uh oh!

aedwardg commented Dec 30, 2020

Uh oh!

NickSchimek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickSchimek commented Dec 30, 2020

Uh oh!

aedwardg commented Dec 30, 2020

Uh oh!

NickSchimek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants