Skip to content

Remove anys and type assertions (#405) #411

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

Closed

Conversation

pshycodr
Copy link
Contributor

@pshycodr pshycodr commented Apr 1, 2025

#405

Description

Removed all instances of any from the codebase, expect the error types where it is seems necessary.

@Martinsos Martinsos requested a review from sodic April 2, 2025 14:07
@sodic
Copy link
Collaborator

sodic commented Apr 4, 2025

Hey @pshycodr. Thanks for the contribution.

From what I can tell, this PR does more than just remove the as and anys. So I'll kindly ask you to stick to the original issue to minimize the change surface.

A good rule of thumb is to avoid mixing refactors and formatting with other changes. See this blog post for all the tips on good PRs: https://mtlynch.io/code-review-love/

Finally, the PR seems to have some weird leftovers. Was this LLM generated, and if so, did you double check the code?

@pshycodr
Copy link
Contributor Author

pshycodr commented Apr 5, 2025

@sodic
apologies for the earlier changes. I have now updated the PR to strictly follow the original issue. and removed all the major code structure and logic changes.
Let me know if any further changes are needed!

@sodic
Copy link
Collaborator

sodic commented Apr 8, 2025

Hey @pshycodr, thanks for the contribution.

Unfortunatley, I don't have time to dig into this PR at the moment.

It seems like the bulk of it was LLM generated (strange comment leftovers, new unexplained type assertions, etc.). I can't trust it enough to merge it without a detailed check. On the other hand, a detailed check will take some time and the issue is not important enough to justify it.

My apologies for not saying this right away.

For future issues, please check whether we're accepting outside contributor help before starting to work on them (for example, first comment on the issue and only start the PR after we give you a go-ahead). This PR was a bad fit for outside contributors because we didn't necessarily just want to add types to untyped values, we potentially wanted to improve them too.

I'm closing for now but promise to give you credit if we end up using your contribution in the future.

@sodic sodic closed this Apr 8, 2025
@pshycodr
Copy link
Contributor Author

pshycodr commented Apr 8, 2025

Totally understood, and thanks for the honest response.
Apologies again for not confirming before picking up the issue, I’ll make sure to align first going forward.
Appreciate the learning and the opportunity to engage , and if any part of the PR helps down the line, I’m glad it added value.
Looking forward to contributing again, the right way this time.
Cheers.

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