linting (automatic and manual) + pre-commit #2

mathematicalmichael · 2021-07-29T06:01:48Z

pip install pre-commit black flake8 and you're off to the races. no commits will ever contain unlinted code again. just set this up on cafelytics yesterday and wanted to practice on another project so I wouldn't forget the process of setting it up... figured I'd multitask and explore your api while doing so.

set up process (after installing the three packages above):
create the pre-commit yaml config file, pieced together for python relevant stuff but not too intrusive, just the basics.
pre-commit install
pre-commit run --all
then manual linting + checking with flake8 after auto-formatting with black.

didn't go so far as to define development dependencies because I didn't want to add a setup.cfg/py file at this stage. think you only need to run pre-commit install once in your local dev environment.

mathematicalmichael · 2021-07-29T06:02:47Z

cannlytics_api/urls.py

-
    # TODO: Build out additional endpoints
-
    # /regulations
-
    # /instruments
-
    # /analytes
-
    # /instruments
-
    # /lab_results
-
    # Optional: Find a way to generalize
    # path("v1/<slug:endpoint>/", views.get_labs, name="endpoint"),
-


ah that's a shame. if you want these back in, I can add comments on the blank lines for placeholders.

keeganskeate · 2021-07-29T15:35:20Z

Awesome for jumping right in @mathematicalmichael, linting is definitely needed. I will review this today.

Just a heads up, I've consolidated the API code (for now) into the main Cannlytics platform. As it's simply a Django app, it can be decoupled and run in a separate container if needed. Simply to speed up development I condensed the code base into 1 repository.

The API is functional for development in this repository, but you may want to work in the main repository for the time being. This repository may be updated in the future to contain a standalone implementation of the API.

Thank you again,
Keegan

mathematicalmichael · 2021-07-29T18:14:51Z

Word! Would you be interested in this kind of PR on the other repo?

I did notice it was more active but didn't know what to make of that. Appreciate the explanation.

keeganskeate · 2021-07-30T01:55:32Z

Definitely, let me explore the linting in this repository first, just don't want to add anything that may trip up the publishing process. This is the next task on my plate.

keeganskeate

Hey Michael I am a fan of linting, so thank you for adding this. There are 2 main project-specific code style choices that have been made that conflict with the style enforced by Black:

Apostrophes are preferred quotations for readability and speed of writing.
A new line at the end of files is preferred to stay consistent with style across file types (i.e. all files should end in a new line, regardless of type). This is inline with the Python philosophy that there should be preferably 1 way to do something and special cases are not special enough to break the rules.

Other than these 2 main conflicts, Black does catch some fixes that need to be made, such as extra spaces, etc.

So, long story short, for my sake it would be a smaller change if you were able to ignore these 2 rules or make a request with specific style fixes, such as removing extraneous spaces.

Please let me know your thoughts and if I'm missing something. I definitely want to get your contribution committed.

Thank you and talk with you soon,
Keegan

mathematicalmichael · 2021-07-31T02:49:19Z

yeahhh I prefer the apostrophes too. I write with them and let the autolinting take care of it as an after thought.
I am not sure if black allows overrides, I think its kind of against its philosophy. I do feel you on that, will look it up. May have to use autopep8 instead perhaps...

mathematicalmichael · 2021-07-31T02:55:11Z

here's their rationale: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html

let me know what you think. I dont care much, I type single ones while developing, and lint at the end.

there does seem to be an option to skip string normalization.

keeganskeate · 2021-07-31T04:51:18Z

Thanks for looking into this. There appears to be an option to ignore single quotes. Perhaps we can try that and see if that would make adopting Black a smaller change. Would you mind exploring this option? Otherwise I can take a look tomorrow. Thank you for exploring this, it is going to be important to standardize the project to help with adoption.

Would you want to explore the option described below if you have time?

If you are adopting Black in a large project with pre-existing string conventions (like the popular “single quotes for data, double quotes for human-readable strings”), you can pass --skip-string-normalization on the command line. This is meant as an adoption helper, avoid using this for new projects.

mathematicalmichael · 2021-08-02T17:13:30Z

@keeganskeate yeah I can try that but what do you think about the philosophy of "type single quotes, develop as you wish, and then let the formatter take care of linting?"

unfortunately I didn't stage the commits in pieces that would make reverting black easy (precommit was configured to not allow me to commit without black passing first, didn't think to disable that and commit that change first... but def noted for future attempts!!!), and did fix a number of linting errors manually. So I just want to make sure you're definitely sure that you want single quotes before re-doing the linting. That would look like: try a find/replace for all quotes to single, re-run black with that flag, and then manually review the diff to see if there were any double quotes that changed incorrectly.

keeganskeate · 2021-08-02T19:49:12Z

Hey @mathematicalmichael , I don't have the strongest preference one way or the other (as long as the code runs!). I do have a slight preference for reading single quotes. I have been waffling over which to adopt, so if you have strong preferences then we can consider picking one.

If you don't mind, then smaller commits would be easier to review. I apologize that you may have to re-do some of the work. A good portion of the linting was helpful, so please see if there is a way that your work can be salvaged and merged in some manner.

So, perhaps start a new branch (not necessarily) and commit a handful of the changes and I can likely get everything merged. My apologies for the slow down, just want to make sure everything can get reviewed and merged into the project smoothly.

Thank you for your hard work.

P.S. I do think you may want to end up working in the main repository before too long, but please keep steamrolling wherever you are most comfortable.

mathematicalmichael · 2021-08-03T03:56:50Z

@keeganskeate ran those sed statements and seems like there are some mixed usages left over. will comment in review.

mathematicalmichael · 2021-08-03T03:57:22Z

cannlytics_api/api/auth.py

-    """ Identify the user's Firebase account using an ID token. """
-    authorization = request.headers["Authorization"]
+    """Identify the user's Firebase account using an ID token."""
+    authorization = request.headers['Authorization']


@keeganskeate preference?

mathematicalmichael · 2021-08-03T03:58:49Z

tests/test_users_endpoints.py


 Resources:

 """
 import os
 import requests

-BASE = "http://127.0.0.1:4200/"
+BASE = 'http://127.0.0.1:4200/'


Suggested change

BASE = 'http://127.0.0.1:4200/'

BASE = "http://127.0.0.1:4200/"

mathematicalmichael · 2021-08-03T03:59:10Z

tests/test_labs_endpoints.py

+BASE = 'http://127.0.0.1:4200/'
+ENDPOINTS = ['', '/labs']


Suggested change

BASE = 'http://127.0.0.1:4200/'

ENDPOINTS = ['', '/labs']

BASE = "http://127.0.0.1:4200/"

ENDPOINTS = ["", "/labs"]

@keeganskeate how do you want to handle this?

mathematicalmichael · 2021-08-03T03:59:53Z

public/sitemap.xml

Suggested change

</urlset>

</urlset>

mathematicalmichael · 2021-08-03T04:00:03Z

public/robots.txt

Suggested change

Sitemap: https://www.api.cannlytics.com/sitemap.xml

Sitemap: https://www.api.cannlytics.com/sitemap.xml

mathematicalmichael · 2021-08-03T04:00:15Z

cloudmigrate.yaml

Suggested change

# "--", "python", "manage.py", "collectstatic", "--no-input"]

# "--", "python", "manage.py", "collectstatic", "--no-input"]

mathematicalmichael · 2021-08-03T04:01:05Z

utils/firebase.py



 def create_reference(database, reference):
    """Create a database reference for a given path."""
    ref = database
-    parts = reference.split('/')
+    parts = reference.split("/")


Suggested change

parts = reference.split("/")

parts = reference.split('/')

mathematicalmichael · 2021-08-03T04:01:17Z

utils/firebase.py

@@ -31,7 +31,7 @@ def create_reference(database, reference):

 def get_keywords(string):
    """Get keywords for a given string."""
-    keywords = string.lower().split(' ')
+    keywords = string.lower().split(" ")


Suggested change

keywords = string.lower().split(" ")

keywords = string.lower().split(' ')

mathematicalmichael · 2021-08-03T04:01:55Z

utils/firebase.py

+            collection = collection.where(
+                filter["key"], filter["operation"], filter["value"]
+            )
    if order_by and desc:
-        collection = collection.order_by(order_by, direction='DESCENDING')
+        collection = collection.order_by(order_by, direction="DESCENDING")


@keeganskeate mixed usage again.
Do you want these single or double?

mathematicalmichael · 2021-08-03T04:02:09Z

utils/firebase.py



 def create_account(name, email, notification=True):
    """
    Given user name and email, create an account if the email isn't being used
    by an existing account.
    """
-    chars = 'abcdefghijklmnopqrstuvwxyz0123456789!@#$-_'
+    chars = "abcdefghijklmnopqrstuvwxyz0123456789!@#$-_"


Suggested change

chars = "abcdefghijklmnopqrstuvwxyz0123456789!@#$-_"

chars = 'abcdefghijklmnopqrstuvwxyz0123456789!@#$-_'

mathematicalmichael · 2021-08-03T04:02:53Z

utils/firebase.py

-    print(
-        "File {} uploaded to {}.".format(
-            source_file_name, destination_blob_name
-        )
-    )
-


I actually prefer this older style but black does not... =/

mathematicalmichael · 2021-08-03T04:04:53Z

cannlytics_api/api/base.py

+    message = 'Welcome to the Cannlytics API.'
+    message += f'The current version is {VERSION} and is located at {BASE}/{VERSION}.'
+    return Response({'data': message}, content_type='application/json')


preference here?

mathematicalmichael · 2021-08-03T04:05:57Z

cannlytics_api/api/base.py

+        message += f'{endpoint}\n'
+    return Response({'data': message}, content_type='application/json')


Suggested change

message += f'{endpoint}\n'

return Response({'data': message}, content_type='application/json')

message += f"{endpoint}\n"

return Response({"data": message}, content_type="application/json")

mathematicalmichael · 2021-08-03T04:06:12Z

cannlytics_api/api/base.py

+    message = f'Welcome to {VERSION} of the Cannlytics API.'
+    message += 'Available endpoints:\n\n'


Suggested change

message = f'Welcome to {VERSION} of the Cannlytics API.'

message += 'Available endpoints:\n\n'

message = f"Welcome to {VERSION} of the Cannlytics API."

message += "Available endpoints:\n\n"

mathematicalmichael · 2021-08-03T04:08:08Z

review leaves a lot of clarifications required. feel free to commit suggestions.

mathematicalmichael · 2021-08-03T04:09:57Z

cannlytics_api/api/labs.py

        limit = request.query_params.get("limit", None)
        order_by = request.query_params.get("order_by", "state")
        # TODO: Get any filters from dict(request.query_params)
-        labs = get_collection('labs', order_by=order_by, limit=limit, filters=[])
-        return Response({ "data": labs}, content_type="application/json")
+        labs = get_collection("labs", order_by=order_by, limit=limit, filters=[])


Suggested change

labs = get_collection("labs", order_by=order_by, limit=limit, filters=[])

labs = get_collection('labs', order_by=order_by, limit=limit, filters=[])

mathematicalmichael · 2021-08-03T04:12:26Z

cannlytics_api/api/labs.py

@@ -29,17 +29,19 @@ def labs(request, format=None):
        limit = request.query_params.get("limit", None)
        order_by = request.query_params.get("order_by", "state")
        # TODO: Get any filters from dict(request.query_params)
-        labs = get_collection('labs', order_by=order_by, limit=limit, filters=[])
-        return Response({ "data": labs}, content_type="application/json")
+        labs = get_collection("labs", order_by=order_by, limit=limit, filters=[])


usage on line 90 uses double quotes, assuming that was the intent. leaving this

mathematicalmichael · 2021-08-03T04:12:48Z