Skip to content

Conversation

@huacchob
Copy link

Adding some documentation to the code. Would like to add more, but I believe this is a good spot to ask for feedback on changes or suggestions

@jtdub jtdub requested review from aedwardstx and jtdub April 20, 2025 00:52

for line in lines:
stripped_line = line.strip()
stripped_line: str = line.strip()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I typically don't gratuitously add type annotations to variables unless there's some ambiguity pointed out by pyright/mypy --strict. Is there an argument for this practice?

Copy link
Author

Choose a reason for hiding this comment

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

I add type annotations to variables to be explicit. It aides the IDE and shows intent, so developers know to keep this variable as this same or compatible type. I understand why omitting type annotation here would be preferred. I can remove this if this is the practice you decide to keep


[tool.ruff.lint.per-file-ignores]
"**/tests/*" = ["PLC2701", "S101"]
"**/tests/*" = ["PLC2701", "S101"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add back the new line at the end of the file

Copy link
Author

@huacchob huacchob May 20, 2025

Choose a reason for hiding this comment

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

I didn't remove the new line here, in my IDE I can see a trailing new line in pyrpoject.toml. I'm not entirely sure why this diff was found

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