Skip to content

Linting fixes #17

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

Merged
merged 20 commits into from
May 15, 2025
Merged

Linting fixes #17

merged 20 commits into from
May 15, 2025

Conversation

hameerabbasi
Copy link
Contributor

  • Add pre-commit hook for mypy to catch typing errors
  • Linting fixes for entire library.

@hameerabbasi hameerabbasi marked this pull request as ready for review May 13, 2025 04:29
Copy link
Contributor

@willow-ahrens willow-ahrens left a comment

Choose a reason for hiding this comment

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

this is great, thanks! I especially like the match case that was introduced.

Copy link
Member

@mtsokol mtsokol left a comment

Choose a reason for hiding this comment

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

Posted some comments. Let's rebase it and solve conflicts and squash to single commit - the diff should be smaller.

@hameerabbasi hameerabbasi requested a review from mtsokol May 14, 2025 11:49
@hameerabbasi
Copy link
Contributor Author

squash to single commit

This can be done by the squash-merge strategy in GitHub. Also, squashing merge-commits is hard.

@mtsokol
Copy link
Member

mtsokol commented May 14, 2025

squash to single commit

This can be done by the squash-merge strategy in GitHub.

Sure!

Also, squashing merge-commits is hard.

Right, that's why I personally prefer to always git rebase -i main over merging main into the branch.

@willow-ahrens
Copy link
Contributor

Is there a repo-wide setting I need to update here?

@mtsokol
Copy link
Member

mtsokol commented May 14, 2025

Is there a repo-wide setting I need to update here?

Let's make this a principle we stick to - I don't think there is.

[EDIT] Looks that here's a branch rule for it:
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-linear-history

But some people complain about it: https://github.com/orgs/community/discussions/61107#discussioncomment-6671872 although I'm not sure this applies to us.

@hameerabbasi
Copy link
Contributor Author

Right, that's why I personally prefer to always git rebase -i main over merging main into the branch.

I usually prefer that too -- but I do make exceptions for large PRs where one merge conflict is easier to handle than many small ones.

Copy link
Member

@mtsokol mtsokol left a comment

Choose a reason for hiding this comment

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

@hameerabbasi I finished reviewing it: I'm only concerned about LogicNode class and node.py changes (I still prefer the have LogicNode class used as a main node representation).

The rest of the changes LGMT.

@hameerabbasi hameerabbasi requested a review from mtsokol May 15, 2025 11:32
@mtsokol mtsokol merged commit c0cf990 into finch-tensor:main May 15, 2025
5 checks passed
@mtsokol
Copy link
Member

mtsokol commented May 15, 2025

Merged, Thanks @hameerabbasi!

@hameerabbasi hameerabbasi deleted the lint-fixes branch May 15, 2025 11:59
willow-ahrens added a commit that referenced this pull request May 15, 2025
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.

3 participants