Skip to content
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

add easy type hints to some function returns #1426

Merged
merged 4 commits into from
Nov 10, 2024
Merged

Conversation

castedo
Copy link
Contributor

@castedo castedo commented Nov 10, 2024

In the process of stating to use Dulwich (for https://gitlab.com/perm.pub/hidos/-/commits/sshsig), I added the following type hints to get mypy to pass with strict setting (in hidos).

Seems like a very safe merge.

I'll probably have some more type hints in the next few days. I'll ping you when I think I don't have any more type hints to add this week.

@castedo castedo requested a review from jelmer as a code owner November 10, 2024 11:39
dulwich/refs.py Outdated Show resolved Hide resolved
dulwich/refs.py Outdated Show resolved Hide resolved
@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

@jelmer regarding the failing mypy check ... I can either take out the type hints I added with TreeEntry or I can attempt some further type changes to resolve the type inconsistencies in the code regarding TreeEntry and in_path.

I'm thinking for now I will keep this PR "easy" and take out the TreeEntry type hints and create a separate PR and discussion on these TreeEntry and in_path type inconsistencies. I didn't realize I was starting to open a can of worms. 😅 So I'll try and put the worms in a different PR.

Unless you prefer a bigger less easy wormy PR.

@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

OK, I took on the type hint worms with TreeEntry and ran mypy on ubuntu and it passes now. I'll cycle back to you regarding the TreeEntry.in_path issue.

* fix mistype in call to TreeEntry.in_path
* add back the earlier type hints using TreeEntry
@jelmer jelmer self-requested a review November 10, 2024 15:52
@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

OK, i looked more into the mypy issue and the 3rd commit is easy and safe enough to go ahead with the extra hint using TreeEntry.

@jelmer
Copy link
Owner

jelmer commented Nov 10, 2024

@jelmer regarding the failing mypy check ... I can either take out the type hints I added with TreeEntry or I can attempt some further type changes to resolve the type inconsistencies in the code regarding TreeEntry and in_path.

I'm thinking for now I will keep this PR "easy" and take out the TreeEntry type hints and create a separate PR and discussion on these TreeEntry and in_path type inconsistencies. I didn't realize I was starting to open a can of worms. 😅 So I'll try and put the worms in a different PR.

Unless you prefer a bigger less easy wormy PR.

That sounds reasonable. In general I prefer smaller but self-contained PRs that can land independently.

@jelmer jelmer enabled auto-merge November 10, 2024 15:54
@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

Now that 3.9 is the minimum I'm tempted to explore the possibility of converting TreeEntry to a dataclass and enable more type checking. But I should focus first on this sshsiglib work and getting hidos using Dulwich.

@jelmer
Copy link
Owner

jelmer commented Nov 10, 2024

Ptal the ruff failures

@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

Ptal the ruff failures

Oh man, I'm having trouble getting my dev env up to standards here! 😅

I'll eventually get my dev container to be close to these CI checks!

auto-merge was automatically disabled November 10, 2024 16:04

Head branch was pushed to by a user without write access

@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

OK, now I've got ruff installed and will use to pre-check.

So do you want me to squash this chain of slightly confused commits?

@jelmer
Copy link
Owner

jelmer commented Nov 10, 2024

Squashing would be preferred, but I can also do that at merge time if the history isn't sensible.

@castedo
Copy link
Contributor Author

castedo commented Nov 10, 2024

Squashing would be preferred, but I can also do that at merge time if the history isn't sensible.

OK, I gotta run off until after you're probably asleep. I'll squash late tonight if you don't beat me to it.

@jelmer jelmer merged commit 6678130 into jelmer:master Nov 10, 2024
24 checks passed
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.

2 participants