RM-17-fix-flake-8-violations #182
Conversation
transitioned from calling np.asscalar to calling the cell's .item method
Update expected column types from TIMESTAMPTZ to TIMESTAMP WITH TIME ZONE.
…sts' into RM-17-fix-flake-8-violations
|
These are all the "low lift" flake violations. The other ones are of the "{foo} is too complex" sort which I feel like can be handled in another PR. |
| @@ -83,7 +83,7 @@ def set_grant_permissions_for_users(self, schema_name: str, table: str, | |||
| def supports_time_type(self) -> bool: | |||
| return True | |||
|
|
|||
| def type_for_date_plus_time(self, has_tz: bool=False) -> sqlalchemy.sql.sqltypes.DateTime: | |||
| def type_for_date_plus_time(self, has_tz: bool = False) -> sqlalchemy.sql.sqltypes.DateTime: | |||
There was a problem hiding this comment.
From what I've seen so far the original codebase more commonly uses the first option, with no extra space (my personal preference as well), but not a big deal.
There was a problem hiding this comment.
Agreed stylistically but triggering flake8 so 🤷
| @@ -15,7 +15,7 @@ def prep_and_load(tbl: TargetTableDetails, | |||
| load: Callable[[DBDriver], Optional[int]], | |||
| load_exception_type: Type[Exception], | |||
| reset_before_reload: Callable[[], None] = lambda: None) -> MoveResult: | |||
| logger.info(f"Connecting to database...") | |||
| logger.info("Connecting to database...") | |||
There was a problem hiding this comment.
prefer single quotes maybe?
There was a problem hiding this comment.
I've never gotten a good call on which to use and don't care much but... preserving the formatting here and also this resource recommends that, generally, use doubles for sentences because you might add a contraction, i.e. "You've got an error!" and bork your single quotes 🤷 :
https://docs.ckan.org/en/ckan-2.1.5/python-coding-standards.html#use-single-quotes
| @@ -104,7 +104,7 @@ def _get_service(self) -> SheetsService: | |||
| def as_json_serializable(self, cell: Any) -> Any: | |||
| if isinstance(cell, np.generic): | |||
| # MyPy complains that this method does not exist | |||
| native = np.asscalar(cell) # type: ignore | |||
| native = cell.item() | |||
There was a problem hiding this comment.
can't add this comment at the exact line but it seems like there should be a way to use numpy.ndarray.tobytes or something similar to serialize this instead of the nested list comprehension calling this method down below.
There was a problem hiding this comment.
definitely agree; this is actually just a change I pulled forward from the RM-7-update-default-python-image-for-integration-tests-to-3-9-remove-build-tests-for-deprecated-python-version branch where there were so many errors on transition that I was trying to keep changes as minimal as possible.
But, if you're willing to dive deep and rewrite this call, just create a ticket for it and it'll be a good way to get to know the tool better! We'll fix the failing tests first but then the world is our oyster. =)
| @@ -1 +1 @@ | |||
| 166 | |||
| 9 | |||
There was a problem hiding this comment.
I don't exactly understand how these are used but why is this such a large numerical change?
There was a problem hiding this comment.
So these "high water marks" are the number of violations there were of a particular protocol. In this case, we had 166 Flake8 violations and brought them down to 9. I mostly did this through using autopep8 then hand correcting some of the other errors. We still have 9 flake8 errors remaining... but those are from "too high complexity" which is more than I wanted to take on in a single PR.
Fix Flake8 violations to make future development easier