dolfies / discord.py-self

A fork of the popular discord.py for user accounts.
https://discordpy-self.rtfd.io/en/latest/
MIT License
651 stars 154 forks source link

Refactor build number extraction logic for robustness #600

Closed idkrn123 closed 8 months ago

idkrn123 commented 8 months ago

This commit refactors the _get_build_number function to improve its robustness and error handling. The updated logic now includes a regex search for the build number within the JavaScript file content, rather than relying on a fixed character offset. This change addresses a potential issue where the build number's position could vary, leading to incorrect extraction. The function now also has a single return point, enhancing readability and maintainability. A default build number is returned in case of any exceptions, ensuring that the function always provides a value.

Summary

This pull request enhances the reliability of the _get_build_number function by implementing a regex-based search for the Discord client build number and improving error handling. It ensures that a valid build number is always returned. Fixes #597.

General Info

pizzaserved commented 8 months ago

@exec thanks for taking care of this. love would this completely replace #596 ?

pizzaserved commented 8 months ago

how about #596 ? edit: actually meant #598 as I've already mentioned 569. but I see there were many PRs on this

idkrn123 commented 8 months ago

@exec thanks for taking care of this. love would this completely replace #596 ?

Yes, this PR seems to be an alternative to #596; however, one that may better resolve the issue.

The occurrence of the unhandled exception users have been experiencing is due to two primary factors: changes in the build number format within the JavaScript files and insufficient exception handling in the extraction function. My understanding is that should the extraction process fail, the function is intended to default to a predetermined build number. In my solution, this number is set to 244594.

Moreover, the revised approach includes a thorough regex search pattern applicable to all JavaScript files accessible from the login page and blanket exception handling to cover any unforeseen errors:

except Exception as e:
    print(f'An unexpected error occurred while fetching the build number: {e}')

I want to clarify that my intention in proposing this pull request was solely to offer a viable solution to the problem at hand and not to overstep or disregard the contributions of others.

dolfies commented 8 months ago

Thank you for taking the time to refactor this. I won't be merging it as I need to refactor this anyway and I'll end up replacing anything that touches aiohttp once the refactor/tls branch is complete. Appreciate the contribution!

pizzaserved commented 8 months ago

@exec thanks for taking care of this. love would this completely replace #596 ?

Yes, this PR seems to be an alternative to #596; however, one that may better resolve the issue.

The occurrence of the unhandled exception users have been experiencing is due to two primary factors: changes in the build number format within the JavaScript files and insufficient exception handling in the extraction function. My understanding is that should the extraction process fail, the function is intended to default to a predetermined build number. In my solution, this number is set to 244594.

Moreover, the revised approach includes a thorough regex search pattern applicable to all JavaScript files accessible from the login page and blanket exception handling to cover any unforeseen errors:

except Exception as e:
    print(f'An unexpected error occurred while fetching the build number: {e}')

I want to clarify that my intention in proposing this pull request was solely to offer a viable solution to the problem at hand and not to overstep or disregard the contributions of others.

thanks, I really appreciate this. I'm currently using it in my fork without issues.

Thank you for taking the time to refactor this. I won't be merging it as I need to refactor this anyway and I'll end up replacing anything that touches aiohttp once the refactor/tls branch is complete. Appreciate the contribution!

thanks. is the intention to eventually fix it separately as part of a bigger refactoring phase, i.e. in a more "wholistic" way? edit: nvm, saw another one was merged

dolfies commented 8 months ago

thanks. is the intention to eventually fix it separately as part of a bigger refactoring phase, i.e. in a more "wholistic" way? edit: nvm, saw another one was merged

Yes, I went with the quick fix for now but eventually this will be refactored as I want to add functionality for specifying your own client properties (as well as tests for stuff like this).