ROCm / rocprofiler-compute

Advanced Profiling and Analytics for AMD Hardware
https://rocm.docs.amd.com/projects/omniperf/en/latest/
MIT License
138 stars 49 forks source link

Update Python format checker #471

Closed coleramos425 closed 2 weeks ago

coleramos425 commented 2 weeks ago

This PR:

Note that the "Formatting" check on this PR is currently failing. This is because isort has caught un-formatted import statements that require formatting. I'm leaving that re-formatting out of this PR to keep things neat. That re-formatting can happen in a separate PR.

Proposed pre-commit hook usage

python3 -m pip install pre-commit
cd rocprofiler-compute
pre-commit install

Now, moving forward any commits you propose to the repository will be automatically check via the pre-commit hook image

dgaliffiAMD commented 2 weeks ago

Cool! Maybe also add your setup instructions to the CONTRIBUTING.md?

dgaliffiAMD commented 2 weeks ago

It looks like your changes are already catching a number of issues with the import sorting. Will you be patching them in this PR too? Or, will that be separate?

coleramos425 commented 2 weeks ago

It looks like your changes are already catching a number of issues with the import sorting. Will you be patching them in this PR too? Or, will that be separate?

What would you recommend? Initially, my thought was to leave those changes out to make the review process easier. But, if things look good and it would be easier, we can lob those changes into this PR too.

dgaliffiAMD commented 2 weeks ago

It looks like your changes are already catching a number of issues with the import sorting. Will you be patching them in this PR too? Or, will that be separate?

What would you recommend? Initially, my thought was to leave those changes out to make the review process easier. But, if things look good and it would be easier, we can lob those changes into this PR too.

Let's fix them, so we have a clean status check for your PR. It should be easy. I think python3 -m isort . should catch and fix everything.

coleramos425 commented 2 weeks ago

Done. Hook deployed. All checks now passing

image