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

OD-1730: Use Ruff for linting and formatting #74

Merged
merged 3 commits into from
Nov 4, 2023

Conversation

hlieberman
Copy link
Contributor

Note: this is blocked by and includes #73. It should not be merged until after that has; setting to WIP to block it for now.

This also grandfathers in the current errors.
@hlieberman hlieberman marked this pull request as ready for review November 1, 2023 14:54
@ariana-paris ariana-paris changed the title WIP: Start using Ruff OD-1730: Use Ruff for linting and formatting Nov 3, 2023
Copy link
Collaborator

@ariana-paris ariana-paris left a comment

Choose a reason for hiding this comment

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

Thank goodness for the Hide whitespace option 😆
I've approved but not merged as I made a couple of comments but nothing blocking, so it's just in case you want to review them. I suspect the massive SQL blocks we have in places will look wonky with the new indentation but that can be addressed if it bothers whoever works on those files in future. On the whole, the changes make sense and tidy things up nicely!

)

# Database settings
parser.add_argument(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aw, RIP my "clever" manual table format 😆 (but I don't imagine many people use this as a reference for the parameters anymore now we use a config file instead)


def _value(self, row):
value = []
for item in row:
if type(item) is str:
if type(item) is str: # noqa: E721
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would isinstance() work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! That's actually the right fix; I just didn't want to mix whitespace only changes with substantive ones in this patch, since it's hard enough to understand already. I'll make another PR to fix this one and the other isinstance underneath.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense 👍


tag_columns = (
tag_col_lookup.keys()
) # [d['col'] for d in tag_col_lookup if 'col' in d]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not formatting related. Just noting the presence of some commented out code here in case anyone wants to remove it

for val in re.split(r", ?", story_tags_row[col]):
if val != "":
if (
type(tag_col_lookup[col]) is str # noqa: E721
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same musing about isinstance as previously

shared_python/Tags.py Outdated Show resolved Hide resolved
xx-Remove-emails-from-Open-Doors-Tables.py Outdated Show resolved Hide resolved
git diff -w is highly recommended for this patch; the only substantive
change is in the .github directory, adding the CI check for formatting
correctness.  The rest is the initial restyling.
Specifically, this targets E701 (if head and body on same line, fixed
already by ruff format), E721 (not using isinstance).
@ariana-paris ariana-paris merged commit bdc5d9d into otwcode:master Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants