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

Aspirational lints #264

Closed
wants to merge 23 commits into from
Closed

Aspirational lints #264

wants to merge 23 commits into from

Conversation

Yoshanuikabundi
Copy link
Contributor

These are lints I'd like the codebase to one day pass. I expect they don't pass now. Mostly stolen from Interchange.

Yoshanuikabundi and others added 2 commits May 29, 2023 18:08
These are lints I'd like the codebase to one day pass.
@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #264 (647041d) into main (0ca1eb4) will decrease coverage by 0.04%.
The diff coverage is 99.09%.

Additional details and impacted files

@mattwthompson
Copy link
Member

This is super duper in my wheelhouse; if you'd like feedback at any point I'm happy to oblige

@Yoshanuikabundi
Copy link
Contributor Author

Thanks Matt, will let you know - I just saw the trailing commas pre-commit on Interchange and was like "I WANT THAT" 😆

If you have feedback in mind I obvs want to hear it but I'm assuming you know that and are offering a more formal review at some point, in which case I appreciate it and will let you know 😁

@mattwthompson
Copy link
Member

Hot dang, I did some stuff and it worked

@mattwthompson mattwthompson marked this pull request as ready for review October 19, 2023 23:30
@Yoshanuikabundi
Copy link
Contributor Author

Holy crap go @mattwthompson!

Are there any change or commits or files that have been manually edited, or is this all automatic changes from lints? It looks amazing! I guess you dropped interrogate coz we're a long way off being full of docstrings?

@mattwthompson
Copy link
Member

Some of the documentation changes in 26d9f53 are manual, but I tried to make no or minimal changes to the source code. I think my likely errors here, if any, or the sort of things to look out if it were possible to review this, are inaccurate statements in some docstrings. Given that (to my surprise) tests are passing I think the chances I broke something are low; adding a type checker is arguably a better way to review this than reading so many LOC.

I guess you dropped interrogate coz we're a long way off being full of docstrings?

I dropped it because I forgot why I had it in Interchange in the first place. We could add it back in and see what happens - pydocstyle complains when anything public lacks a docstring, so I bet the coverage is actually quite good.

@j-wags j-wags marked this pull request as draft March 27, 2024 19:50
@mattwthompson
Copy link
Member

This was fun to work on but it doesn't seem valued - if anybody wishes to pick this up, I'm leaving the branch dangling on the remote

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.

None yet

2 participants