Skip to content

Conversation

sidamd
Copy link
Collaborator

@sidamd sidamd commented Jun 16, 2025

added mypy to pre-commit hooks and dependencies list. Fixed dataanalyzer interface, task.py, event.py, tablesummary.py, and cmd_analyzer.py to pass mypy type checks.

Copy link
Collaborator

@landrews-amd landrews-amd left a comment

Choose a reason for hiding this comment

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

looks good, added a few suggestions.

@sidamd sidamd force-pushed the feature/mypy-precommit branch from b7cf814 to 50255ed Compare June 20, 2025 17:58
@sidamd sidamd requested a review from landrews-amd June 20, 2025 20:16
@sidamd sidamd requested a review from alexandraBara July 24, 2025 19:17
@sidamd
Copy link
Collaborator Author

sidamd commented Jul 24, 2025

Ignored mypy errors on the existing files that were throwing errors

Copy link
Collaborator

@landrews-amd landrews-amd left a comment

Choose a reason for hiding this comment

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

I do not think that we should be adding so many modules to the ignores. If there are type issues in these modules they should be addressed prior to merge.

pyproject.toml Outdated
@@ -57,4 +58,4 @@ profile = "black"

[tool.ruff.lint]
select = ["F", "B", "T20", "N", "W", "I", "E"]
ignore = ["E501", "N806"]
ignore = ["E501", "N806", "B010"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we adding this to the ignores?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ruff has an issue with one of the mypy changes I made, I think it was the Union error so Ruff would fail as well with the mypy change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sidamd can you please double check where this B010 is being thrown and try to rework the code so its not thrown. I looked up the error code and its related to a redundant boolean operation which sounds like it can be fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexandraBara

https://github.com/amd/node-scraper/blob/312e91b94cbf81eacbba3544dec4aec44d672ba9/nodescraper/interfaces/dataanalyzertask.py#L121C6-L122C6

Line 122 of the dataanalyzer file ruff has an error where it prefers direct assignment of a variable. But if I use direct assignment of a variable then mypy will have an issue that I using direct assignment of a variable. I can remove the ignore in the pyproject and just add a comment to ignore the ruff error on line 122 of the dataanalyzer file if you guys prefer that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes this sounds reasonable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@landrews-amd I removed the ruff ignore. Just added the ruff ignore comment to the dataanalyzertask file.

@alexandraBara
Copy link
Collaborator

I do not think that we should be adding so many modules to the ignores. If there are type issues in these modules they should be addressed prior to merge.

I think we should do this incremental. This PR already has many files modified, we can work on the ignored files in future PRs and not post 1 huge PR. I think this was what we had decided initially, to gradually add mypy to files. Since @sidamd enabled mypy, those files will currently fail, so i think ignoring them in this PR is OK.

@sidamd sidamd requested a review from landrews-amd August 18, 2025 20:07
Copy link
Collaborator

@alexandraBara alexandraBara left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants