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

Cleanup unused imports #1803

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Cleanup unused imports #1803

merged 2 commits into from
Dec 4, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Dec 1, 2024

Found two unused imports and an unused variable. From other code, I inferred that you follow the convention to prefix unused (return) variables with underscores.

I noticed that running yapf (as suggested in the contribution guidelines) modified the files in places that I didn't touch. I thus separated the pure formatting changes in the first commit. As argued in PR #1731, I think it would be beneficial (for the project) to enforce formatting as part of the CI. @phschaad not sure if you got to discuss this in the weekly DaCe meeting. I started a discussion page #1804 and I'm happy to contribute a corresponding workflow early next year (assuming we agree).

@romanc romanc changed the title Romanc/unused imports Cleanup unused imports Dec 2, 2024
@romanc
Copy link
Contributor Author

romanc commented Dec 2, 2024

Also: happy to "backport" this to the 1.0 stable branch

Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM.
As for backporting, I am happy to accept a packporting PR to the v1/maintenance branch if you have the time for it!

@phschaad
Copy link
Collaborator

phschaad commented Dec 3, 2024

(Not further going into yapf subject here as we have the discussion ongoing in different forums :) )

@phschaad phschaad added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@phschaad phschaad added this pull request to the merge queue Dec 3, 2024
@romanc romanc mentioned this pull request Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 3, 2024
@phschaad phschaad added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 4, 2024
@phschaad phschaad added this pull request to the merge queue Dec 4, 2024
Merged via the queue into spcl:main with commit 4976e16 Dec 4, 2024
10 checks passed
@romanc romanc deleted the romanc/unused-imports branch December 4, 2024 18:36
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