Skip to content

Conversation

AxelMurilllo
Copy link

  • PR refactors TimeDelta class constructor to simplify time normalization logic with single calculation for total_seconds.
  • Added handling for None types to prevent TypeError for math operations

Previous TimeDelta class constructor:


    def __init__(self, days=0, hours=0, minutes=0, seconds=0):
        _days, _hours = divmod(hours, 24)
        _hours_left, _mins = divmod(minutes, 60)

        self.days = days + _days
        # hours was set to hours again instead of the remainder of divmid(hours, 24)
        self.hours = hours + _hours + _hours_left
        self.minutes = minutes + _mins
        self.seconds = seconds

@AxelMurilllo
Copy link
Author

Merged my branches together but here are my comments so they are easy to find:

  • Added .max_level attribute to uninitiated classes eg: client.get_troop() items
  • parsing fix for "UpgradeTimeM"
    • Edge case for Barbarian 1 -> 2 displayed 0 hours and no UpgradeTimeM data. Barbarian 1 -> 2 has a 5 minute upgrade time.
    • SIDE NOTE: Raged Barbarian in the Builder Base has a 15 second research time from levels 1 -> 2. There is of course no "UpgradeTimeS" in the .csv files so it becomes 0 when parsed and turned into a .json file. This page at coc.guide shows this issue. Hardcode it if you care.
  • APK Scraper as some versions do not have fingerprint.json
    • Running this script now ensures that static asset updates will retrieve with fingerprint.json (unless APKMirror changes something).

@doluk
Copy link
Collaborator

doluk commented May 4, 2025

@AxelMurilllo could you please provide a code example for testing?

@AxelMurilllo
Copy link
Author

Heres a simple script for testing TimeDelta and uninitiated objects .max_level attribute. I used the "Barbarian" as an example as it is the only home village troop with an upgrade time in minutes. Make sure that your static data is updated!

  • EDIT: In the process of testing this script I did discover that the upgrade_times array for uninitiated objects does have issues with the "Raged Barbarian" as it has a 15 second upgrade time. This causes raged_barb.upgrade_times[1] to display the upgrade time for level 2 -> 3 instead of 1 -> 2 and for the upgrade_times array to only have 19 levels instead of 20. I pushed the change to fix this issue. There is no 'UpgradeTimeS' so it just defaults to 0 seconds. Users would have to hardcode this level.
"""Test TimeDelta changes and max_level attribute for uninitiated Barbarian object"""

import asyncio
import os
import unittest
from dotenv import load_dotenv
import coc

class TestBarbarian(unittest.TestCase):
    async def asyncSetUp(self):
        load_dotenv()
        self.client = coc.Client(load_game_data=coc.LoadGameData(always=True))
        await self.client.login(os.getenv("COC_EMAIL"), os.getenv("COC_PASSWORD")) #Edit credential names if necessary
        self.barb = self.client.get_troop("Barbarian")
        self.raged_barb = self.client.get_troop("Raged Barbarian", is_home_village=False)

    async def asyncTearDown(self):
        await self.client.close()

    async def test_barbarian_attributes(self):
        # Test max level
        self.assertEqual(self.barb.max_level, 12, "Barbarian should have max level 12")
        
        # Test upgrade time for level 1 -> 2
        upgrade_time = self.barb.upgrade_time[1]  
        self.assertEqual(upgrade_time.days, 0)
        self.assertEqual(upgrade_time.hours, 0)
        self.assertEqual(upgrade_time.minutes, 30)
        self.assertEqual(upgrade_time.seconds, 0)
        self.assertEqual(upgrade_time.total_seconds(), 1800) 

    async def test_raged_barbarian_attributes(self):
        # Test max level
        self.assertEqual(self.raged_barb.max_level, 20)
        
        # Test upgrade time for level 1 -> 2
        # Known limitation: CSV files don't have seconds attribute!!!
        # Actual time should be 15 seconds but will show as 0
        upgrade_time = self.raged_barb.upgrade_time[1]
        self.assertEqual(upgrade_time.days, 0)
        self.assertEqual(upgrade_time.hours, 0)
        self.assertEqual(upgrade_time.minutes, 0)
        self.assertEqual(upgrade_time.seconds, 0)
        self.assertEqual(upgrade_time.total_seconds(), 0)

if __name__ == "__main__":
    async def run_test():
        test = TestBarbarian()
        await test.asyncSetUp()
        try:
            await test.test_barbarian_attributes()
            await test.test_raged_barbarian_attributes()
        finally:
            await test.asyncTearDown()
    
    asyncio.run(run_test()) 
    print("All tests passed!")

@doluk doluk requested a review from Copilot May 6, 2025 12:08
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the TimeDelta class to simplify its time normalization logic and adds handling for None types. Additionally, it updates the APK download process to use synchronous requests with BeautifulSoup for parsing and adjusts the upgrade_time initialization in the JSON meta loader to match the new TimeDelta API.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
coc/static/update_static.py Updated the APK download function to use requests and improved error handling in download logic.
coc/static/apk_source.py Added a new function to retrieve the direct APK download URL from APKMirror.
coc/miscmodels.py Refactored the TimeDelta class to calculate total seconds in a single calculation.
coc/abc.py Modified upgrade_time initialization to use the updated TimeDelta constructor parameters.
Comments suppressed due to low confidence (1)

coc/abc.py:281

  • If the API provides an 'UpgradeTimeS', consider including it in the TimeDelta initialization to maintain consistency with the previous behavior.
TimeDelta(


# download the APK using the direct URL
print("[+] Downloading APK file...")
response = session.get(direct_url, stream=True)
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

Consider verifying the HTTP response status (e.g., checking response.status_code) before processing the content to ensure the request succeeded.

Suggested change
response = session.get(direct_url, stream=True)
response = session.get(direct_url, stream=True)
response.raise_for_status() # Ensure the request was successful

Copilot uses AI. Check for mistakes.


dl_button = variant_soup.select_one("a.downloadButton")
if not dl_button:
raise Exception("ERROR:Download button not found on variant page")
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

Consider adding a space after 'ERROR:' in the exception message for consistency.

Suggested change
raise Exception("ERROR:Download button not found on variant page")
raise Exception("ERROR: Download button not found on variant page")

Copilot uses AI. Check for mistakes.

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