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

Large refactoring to simplify code and remove duplicate code #13

Merged
merged 13 commits into from
Sep 3, 2024

Conversation

r-hensley
Copy link
Contributor

Hello, thank you for your wonderful library. It's very well made and works well. I've written in the changelog file but I'll write it again here.

I spent some time just consolidating a lot of duplicated code:

  • Every class was defined with duplicate definitions of raw(), __repr__(), and __str__(), so I moved all those to a base class called Object and had the anilist classes derive from that. Additionally, if a class has an ID, I made a Hashable type as a subclass of Object which implemented __eq__() and __hash__().
  • The logic for API calls in every class was duplicated, so I moved it all to one function, api_query() and had all functions call that instead.
  • The data analysis and packaging of the information received from the response was mostly duplicated between the sync_client.py file and the async_client.py for the purpose of having different logic in the api query step, but I moved all the steps that were the same for both files out into a third external file client_process.py so they can be called equally.

One change I made that technically could be breaking is I removed the mass ignoring of exceptions in the data packaging code. There were many instances of code like:

try:
    # code here
except Exception:
    pass

This probably isn't good because it hides errors which can lead to both complications in debugging and unintended behavior in production. For now, I left the structure of the code the same and just changed "pass" to "raise". To undo this change, one can do a substitution operation in client_process.py to replace "raise" back to "pass".


Finally, I fixed what I believe to be an error but I'm technically not sure. In the function get_message_activity()

def get_message_activity(

it looks like there are two functions inside the one definition, and the function starts over with a new query call here:

if not self.httpx:

so I split the function into get_message_activity() and get_message_activity_sent().

https://github.com/r-hensley/python-anilist/blob/f9183a0656d5db8a9da68902f36dc67c355201ab/anilist/sync_client.py#L376-390


I ran all the tests and (after changing one obsolete ID in one of the tests), confirmed they all passed:

C:\Users\ryry0\AppData\Local\Programs\Python\Python310\python.exe "C:\Program Files\JetBrains\PyCharm 2022.1.3\plugins\python\helpers\pycharm\_jb_pytest_runner.py" --path C:/Users/ryry0/Documents/Python/anime_staff_checker/pythonanilist/tests/test_sync.py
Testing started at 11:54 PM ...
Launching pytest with arguments C:/Users/ryry0/Documents/Python/anime_staff_checker/pythonanilist/tests/test_sync.py --no-header --no-summary -q in C:\Users\ryry0\Documents\Python\anime_staff_checker\pythonanilist\tests

============================= test session starts =============================
collecting ... collected 16 items

test_sync.py::test_search[Bakemonogatari-anime] 
test_sync.py::test_search[Bakemonogatari-manga] 
test_sync.py::test_search[Senjougahara-character] 
test_sync.py::test_search[Chiwa Saitou-staff] 
test_sync.py::test_search[travis-user] 
test_sync.py::test_get[5081-anime] 
test_sync.py::test_get[101311-manga] 
test_sync.py::test_get[22037-character] 
test_sync.py::test_get[95061-staff] 
test_sync.py::test_get[travis-user] 
test_sync.py::test_get[travis-list_anime] 
test_sync.py::test_get[travis-list_manga] 
test_sync.py::test_get_list_item 
test_sync.py::test_get_activity[travis-anime] 
test_sync.py::test_get_activity[travis-manga] 
test_sync.py::test_get_activity[travis-text] 

============================= 16 passed in 13.78s =============================

Process finished with exit code 0
C:\Users\ryry0\AppData\Local\Programs\Python\Python310\python.exe "C:\Program Files\JetBrains\PyCharm 2022.1.3\plugins\python\helpers\pycharm\_jb_pytest_runner.py" --path C:/Users/ryry0/Documents/Python/anime_staff_checker/pythonanilist/tests/test_async.py
Testing started at 11:54 PM ...
Launching pytest with arguments C:/Users/ryry0/Documents/Python/anime_staff_checker/pythonanilist/tests/test_async.py --no-header --no-summary -q in C:\Users\ryry0\Documents\Python\anime_staff_checker\pythonanilist\tests

============================= test session starts =============================
collecting ... collected 16 items

test_async.py::test_search[Bakemonogatari-anime] 
test_async.py::test_search[Bakemonogatari-manga] 
test_async.py::test_search[Senjougahara-character] 
test_async.py::test_search[Chiwa Saitou-staff] 
test_async.py::test_search[travis-user] 
test_async.py::test_get[5081-anime] 
test_async.py::test_get[101311-manga] 
test_async.py::test_get[22037-character] 
test_async.py::test_get[95061-staff] 
test_async.py::test_get[travis-user] 
test_async.py::test_get[travis-list_anime] 
test_async.py::test_get[travis-list_manga] 
test_async.py::test_get_list_item 
test_async.py::test_get_activity[travis-anime] 
test_async.py::test_get_activity[travis-manga] 
test_async.py::test_get_activity[travis-text] 

============================= 16 passed in 12.91s =============================

Process finished with exit code 0

Overall, I reduced:

  • sync_client.py: 1,370 lines → 390 lines
  • async_client.py: 1,463 lines → 407 lines
  • client_process.py (NEW): 0 lines → 703 lines

Tell me if you have any questions.

@AndrielFR AndrielFR requested a review from HitaloM July 23, 2023 08:58
@AndrielFR
Copy link
Member

not going to lie, i'm out of time to review this pull request, so i haven't replied yet.

@r-hensley
Copy link
Contributor Author

r-hensley commented Aug 13, 2023

That's totally fine, I made a huge pull request (large rewrite) to an old repo. I'm using my own fork for my purposes so I don't personally need the pull request merger for anything, it's really just more for other people to see the existence of my fork/branch. You can do the actual merge at your own pace.

@AndrielFR
Copy link
Member

AndrielFR commented Aug 14, 2023

That's totally fine, I made a huge pull request (large rewrite) to an old repo. I'm using my own fork for my purposes so I don't personally need the pull request merger for anything, it's really just more for other people to see the existence of my fork/branch. You can do the actual merge at your own pace.

i understand, great, i'm glad for that.

Copy link
Member

@AndrielFR AndrielFR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit old, but not forgotten, thank you for your PR, i appreciate it

@AndrielFR AndrielFR merged commit 9ca961d into AmanoTeam:main Sep 3, 2024
0 of 8 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