Skip to content

Conversation

@aktech
Copy link
Member

@aktech aktech commented Aug 7, 2024

This PR adds a pre-commit lint action. Since there are lots of linting issues when running ruff, this actions only runs linting on the changed files in the PR, until we fix all the linting issues.

In future we can also directly use: https://pre-commit.ci/ which would also auto-commit linting issues. I didn't went with it right away as I couldn't find a way to do a selective run on the changed files with that.

The proposal is to get this in and start linting pull requests and slowly fix linting errors and then start running pre-commit on full code based instead of PR diff.

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

This looks good and I agree, if we turn on e.g. ruff-format right now the entire repo will be hit. Probably best to do in a separate PR, or bit-by-bit.

@aktech
Copy link
Member Author

aktech commented Aug 7, 2024

This PR does turns ruff on, but only on the PR diffs, as in for the commits in the PR (new code basically), without worrying about anything before that.

@gbaned gbaned requested a review from roseayeon August 12, 2024 06:28
@gbaned gbaned self-assigned this Aug 12, 2024
@roseayeon
Copy link
Contributor

roseayeon commented Aug 13, 2024

Thanks for your work on this. However, merging this PR directly into our internal codebase could potentially cause issues with our internal systems. To avoid any disruptions, I'll separate the internal code from the GitHub code before approving the PR. Thank you for your understanding.

@roseayeon roseayeon merged commit a01c22a into tensorflow:master Aug 14, 2024
@aktech aktech deleted the lint-ci branch August 19, 2024 16:18
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.

4 participants