Skip to content

[Lint] Run lint by pre-commit#3239

Closed
nijkah wants to merge 3 commits intohpcaitech:mainfrom
nijkah:run_lint
Closed

[Lint] Run lint by pre-commit#3239
nijkah wants to merge 3 commits intohpcaitech:mainfrom
nijkah:run_lint

Conversation

@nijkah
Copy link
Copy Markdown
Contributor

@nijkah nijkah commented Mar 24, 2023

This PR should be merged after #3238

📌 Checklist before creating the PR

  • I have created an issue for this PR for traceability
  • The title follows the standard format: [doc/gemini/tensor/...]: A concise description
  • I have added relevant tags if possible for us to better distinguish different PRs

🚨 Issue number

fixes #3237

📝 What does this PR do?

Run lint by pre-commit.

for file in ${{ steps.find-changed-files.outputs.all_changed_files }}; do
pre-commit run --files $file || true

Since the current workflow only checks changed files, it does not edit old lines.

💥 Checklist before requesting a review

  • I have linked my PR to an issue (instruction)
  • My issue clearly describes the problem/feature/proposal, with diagrams/charts/table/code if possible
  • I have performed a self-review of my code
  • I have added thorough tests.
  • I have added docstrings for all the functions/methods I implemented

⭐️ Do you enjoy contributing to Colossal-AI?

  • 🌝 Yes, I do.
  • 🌚 No, I don't.

Tell us more if you don't enjoy contributing to Colossal-AI.

@FrankLeeeee
Copy link
Copy Markdown
Contributor

Hi @nijkah , this PR has conflict with the main branch, can you try to resolve the conflict first?

@nijkah
Copy link
Copy Markdown
Contributor Author

nijkah commented Mar 27, 2023

Hi @nijkah , this PR has conflict with the main branch, can you try to resolve the conflict first?

@FrankLeeeee I fixed some conflicts between yapf and isort!

@nijkah
Copy link
Copy Markdown
Contributor Author

nijkah commented Mar 30, 2023

@FrankLeeeee Hi. The errors on the checks don't seem to be due to this PR. This PR is just formatting.
Should I fix it?

@FrankLeeeee
Copy link
Copy Markdown
Contributor

@FrankLeeeee Hi. The errors on the checks don't seem to be due to this PR. This PR is just formatting. Should I fix it?

Hi @nijkah , I suspect that the code formatting broke something. Sometimes, the import order can matter, e.g. in C++ and CUDA files. I would like to suggest to format in a module-by-module way. This can bring two benefits.

  1. We can approach this matter with a finer granularity, easier to spot what led to the CI failure
  2. The amount of code change in a single PR can be reduced, which could make it easier for people who are contributing, e.g. they won't face too many conflicts when pulling a single commit

@FrankLeeeee
Copy link
Copy Markdown
Contributor

If you like, I can handle this together with you. @nijkah

@nijkah
Copy link
Copy Markdown
Contributor Author

nijkah commented Mar 31, 2023

@FrankLeeeee I agree with you. I'll create new PRs based on modules.
Thank you for your suggestion!

@nijkah nijkah closed this Mar 31, 2023
@nijkah nijkah mentioned this pull request Mar 31, 2023
3 tasks
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.

[BUG]: Post-commit workflow does not work!

2 participants