Cleanup the codebase - Githubissues

ItsDrike commented 2 years ago

There are many older code remnants scattered in the project, several non-pythonic implementations for certain things, and approaches which involve too much repetition or have other issues,

This PR is an attempt to clean up the code-base a bit to make it more readable and maintainable.

ItsDrike commented 2 years ago

As a part of this PR, I also wanted to convert at least most bare try-except, or except Exception statements to only except specific exceptions explicitly, since bare exception will also capture things like keyboardinterrupts, etc. If this is desired, it should be marked. Statements like these are a place where a lot of bugs may get captured without any feedback of what happened, if we need to catch some exception in a block of code, it should always be explicit.

To do this, I've handled (or will handle) some of these from what I figured they were supposed to be capturing, so when reviewing please take in mind that these were mostly my assumptions on what these try-except blocks were handling and it may very well be a wrong assumption, so please pay close attention to avoid this PR stopping some error handling which should've been captured. But also make sure I haven't used a way too general exceptions (for example capturing every error belonging to certain library, when only one or a few specific ones would suffice).

While doing this, there were some try-except blocks which I found in the code-base, but wasn't able to identify which exceptions exactly are they supposed to catch. This kind of confusion is exactly why we shouldn't allow any further broad except statements, for example if we need to catch a KeyError, we shouldn't just add a try-except that catches all Exceptions, not to mention all BareExceptions, which was the case in some of these blocks in this repository. Doing that is just a bad practice and makes figuring out what the code is doing very hard sometimes.

This is the list of try-except statements which didn't have any indication of what they're handling and I wasn't easily able to figure out what it is.

I'd first like to get a response on what errors these try-except blocks are supposed to catch so that these can be changed to only catch those errors specifically. So when reviewing, I'd appreciate a comment explaining these before approving and merging.

ItsDrike commented 2 years ago

Note about commit 9b35cef failing pytype:

For some reason, pytype doesn't play nicely with overriding type-hints directly in overridden init. After calling the init from superclass, it basically ignores this new type assignment. Specifically this assignment happens on line 110 in pinger.py and line 63 in querier.py, however these lines themselves isn't causing any issues, rather them being ignored is.

This is because even though the type was clearly set to something else, pytype keeps thinking that self.connection is the non-async version, as it was in the super class, which means anything non-compatible between these connection classes will cause type errors. The only reason this worked before was that the types weren't defined at all (were treated as typing.Any), skipping any type-checking there completely.

I'm not sure how should I go about fixing this, both pyright and mypy are fine with this kind of assignment.

kevinkjt2000 commented 2 years ago

I'm okay with ditching pytype if another tool works correctly. My experience with every python type checker has been that they all have bugs, pros, and cons (kinda like every other piece of software).

I'd first like to get a response on what errors these try-except blocks are supposed to catch so that these can be changed to only catch those errors specifically. So when reviewing, I'd appreciate a comment explaining these before approving and merging.

I don't know what they were either, so of course, I'm going to approve and merge when tests pass :smile:

ItsDrike commented 2 years ago

I'm okay with ditching pytype if another tool works correctly.

I'm willing to replace pytype with pyright, it should result in much faster type checking as pytype is really slow sometimes and I find it a lot more sensible in some places than pytype (at least from what I've seen in my very limited experience with it). Pyright is quite popular and supports most new typing features relatively quickly after they're added and it is maintained by Microsoft.

An alternative would be mypy which is maintained by the python software foundation directly, however I often find mypy way too strict for certain things for no real reason which results in needing way too many ignores and overrides, it's also a bit slower than pyright, though still beats pytype by a long shot. There's also pyre-check and a few other less popular options, but I don't have any experience with those so I can't recommend them.

I don't think a cleanup PR is a place to change type checkers thogh, it may be fine to merge this first, even without passing typing requirements of pytype and fix that with another PR later, that's purely for switching type checkers. Alternatively this PR could simply wait until that PR gets in, though there are some typing fixes and this code does already pass pyright, if this were supposed to be done before this gets merged, it would be a bit annoying as to fulfill pyright many of the same things done here would need to happen in another PR again.

I don't know what they were either, so of course, I'm going to approve and merge when tests pass 😄

Yeah, that's a problem I'd like to solve before this gets merged, wide try-except statements are often a huge problem when debugging, I'll try to find what they're for and replace at least most of them, though I can't promise I won't introduce any bugs, but that's probably still better than keeping it as is since those should get discovered fairly quickly and shouldn't be too hard to fix, and it will make the code a lot easier to debug afterwards.

Additional note: I've left a comment in the resolved conversation on adding tests for the style description parsing, pointing out a possible issue that may be occurring, since you marked as resolved, I assume it's not an issue? Though I'd like a more concrete response on that in case you missed it and just marked it as resolved since the test itself is now there.

kevinkjt2000 commented 2 years ago

Yes, we could save fixing the typing stuff for later. Additionally I could delay creating a release.

Thanks for doing all this! I haven’t been able to donate a lot of time lately to this project.

On Jan 26, 2022, at 7:23 PM, ItsDrike @.***> wrote:

I'm okay with ditching pytype if another tool works correctly.

I'm willing to replace pytype with pyright, it should result in much faster type checking as pytype is really slow sometimes and I find it a lot more sensible in some places than pytype (at least from what I've seen in my very limited experience with it). Pyright is quite popular and supports most new typing features relatively quickly after they're added and it is maintained by Microsoft.

An alternative would be mypy which is maintained by the python software foundation directly, however I often find mypy way too strict for certain things for no real reason which results in needing way too many ignores and overrides, it's also a bit slower than pyright, though still beats pytype by a long shot. There's also pyre-check and a few other less popular options, but I don't have any experience with those so I can't recommend them.

I don't think a cleanup PR is a place to change type checkers thogh, it may be fine to merge this first, even without passing typing requirements of pytype and fix that with another PR later, that's purely for switching type checkers. Alternatively this PR could simply wait until that PR gets in, though there are some typing fixes and this code does already pass pyright, if this were supposed to be done before this gets merged, it would be a bit annoying as to fulfill pyright many of the same things done here would need to happen in another PR again.

I don't know what they were either, so of course, I'm going to approve and merge when tests pass 😄

Yeah, that's a problem I'd like to solve before this gets merged, wide try-except statements are often a huge problem when debugging, I'll try to find what they're for and replace at least most of them, though I can't promise I won't introduce any bugs, but that's probably still better than keeping it as is since those should get discovered fairly quickly and shouldn't be too hard to fix, and it will make the code a lot easier to debug afterwards.

Additional note: I've left a comment in the resolved conversation on adding tests for the style description parsing pointing out a possible issue that may be occurring, since you marked as resolved, I assume it's not an issue? Though I'd like a more concrete response on that in case you missed it and just marked it as resolved since the test itself is now there.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.

Dinnerbone / mcstatus

Cleanup the codebase #182