Run pre-commit hooks after inputs redesign to use GenericDataset #143
Conversation
dyastremsky
force-pushed
the
dyas-precommit
branch
from
6a4b6c2
to
212707c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dyastremsky
force-pushed
the
dyas-precommit
branch
from
212707c
to
990842c
Compare
Update hook Update test_cli Remove accidental file Remove unused variables
dyastremsky
force-pushed
the
dyas-precommit
branch
from
990842c
to
4deaa59
Compare
dyastremsky
requested review from
nicolasnoble,
matthewkotila,
lkomali and
ganeshku1
| @@ -39,7 +39,7 @@ jobs: | |||
| fail-fast: false | |||
There was a problem hiding this comment.
In the future, could we put all formatting-related changes in their own commit or even better in their own PR?
There was a problem hiding this comment.
What do you mean? Usually, pre-commit is turned on, so the formatting would happen for every commit. Since it was turned off as part of this development work, re-enabling it immediately ran all the hooks. And then it warned me about types, other static errors, etc., so I fixed those manually, but it wouldn't let me commit without fixing everything.
I could have let the auto-formatting run, then uninstalled pre-commit, pushed a commit with just auto-formatting, re-installed pre-commit, and made any manual changes in a separate commit if that's in line with what you're asking for. Yeah, that sounds like a good strategy if this happens again.
P.S. I agree this is suboptimal and I'd prefer to find ways to avoid disabling pre-commit and creating these kinds of PRs in the future.
There was a problem hiding this comment.
I'm not sure how to achieve it, but I mean literally what I said. I had to read everything way closer because I couldn't tell if it was going to be a formatting related change or some other kind of change (e.g. using a python 3.10 feature or something).
It would be much easier to review if a single commit contained anything related to auto-formatting. Like run precommit without any feature-related changes for the first commit, then future commits are the non-formatting changes, like adding a trialing comma or whatever. That way, as the reviewer, I can skip looking at the commit/changes that auto-format, which saves mental load, and because I trust whatever auto-formatter we're using, and we have testing to make sure auto-formatting doesn't accidentally break anything.
There was a problem hiding this comment.
Totally fair. I had originally had it as a separate commit and will do so next time.
Just a heads up that I didn't make the aforementioned change. The trialing comma was actually pre-commit formatting. The only things I did were fixing the errors pre-commit returned, then fixing the 2-3 CLI unit tests that broke when the behavior was fixed. I think you're right though - I can run it just with auto-format and save that as a commit so that you don't need to review that commit.
Thanks again for reviewing so much with such quick turnaround, Matt!
|
Looks good overall, just some small questions. |
Re-run pre-commit hooks after inputs redesign. Since the new analyze changes will only support Python 3.10+, updating the code here to also only support 3.10+ to use aliases.