Refactor and update structure #20

2025-10-05T19:32:38+02:00

Berack96 commented

2025-10-05 19:32:38 +02:00

(Migrated from github.com)

Update agents and models, reorganize extraction functions, and enhance import management. Improve documentation, type checks, and README. Refactor architecture by relocating core classes and updating import paths for better maintainability.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-10-05 19:33:32 +02:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR implements a comprehensive refactoring to improve code structure, enhance type safety, and update the architecture. The changes reorganize core classes, update import paths, and establish a cleaner separation of concerns between agents, base classes, utilities, and API wrappers.

Key changes:

Refactored architecture by moving core classes to dedicated base modules and agents directory
Enhanced type annotations throughout the codebase for better type safety
Reorganized import structure and consolidated utility functions

Reviewed Changes

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

Show a summary per file

File	Description
tests/	Updated imports and type annotations, fixed test references to renamed classes and methods
src/app/utils/	Enhanced type safety in wrapper handler, simplified chat manager, consolidated utility imports
src/app/social/	Updated imports and function names, improved type annotations
src/app/news/	Refactored imports and standardized extraction function naming
src/app/markets/	Updated base class references and improved type annotations
src/app/base/	Restructured base classes with enhanced type safety and better timestamp handling
src/app/agents/	New directory structure with models, predictor, team, and pipeline organization
README.md	Updated documentation with improved structure and installation instructions
Dockerfile	Improved Docker configuration for better compatibility

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

## Pull Request Overview This PR implements a comprehensive refactoring to improve code structure, enhance type safety, and update the architecture. The changes reorganize core classes, update import paths, and establish a cleaner separation of concerns between agents, base classes, utilities, and API wrappers. Key changes: - Refactored architecture by moving core classes to dedicated base modules and agents directory - Enhanced type annotations throughout the codebase for better type safety - Reorganized import structure and consolidated utility functions ### Reviewed Changes Copilot reviewed 37 out of 39 changed files in this pull request and generated 4 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | tests/ | Updated imports and type annotations, fixed test references to renamed classes and methods | | src/app/utils/ | Enhanced type safety in wrapper handler, simplified chat manager, consolidated utility imports | | src/app/social/ | Updated imports and function names, improved type annotations | | src/app/news/ | Refactored imports and standardized extraction function naming | | src/app/markets/ | Updated base class references and improved type annotations | | src/app/base/ | Restructured base classes with enhanced type safety and better timestamp handling | | src/app/agents/ | New directory structure with models, predictor, team, and pipeline organization | | README.md | Updated documentation with improved structure and installation instructions | | Dockerfile | Improved Docker configuration for better compatibility | </details> --- **Tip:** Customize your code reviews with copilot-instructions.md. <a href="/Berack96/upo-appAI/new/main/.github?filename=copilot-instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Create the file</a> or <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">learn how to get started</a>.

src/app/base/markets.py

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 19:33:32 +02:00

The asset_id parameter lacks a default value, which is inconsistent with the previous API design and could break existing code that relied on the default 'BTC' value.

    def get_historical_prices(self, asset_id: str = "BTC", limit: int = 100) -> list[Price]:

The asset_id parameter lacks a default value, which is inconsistent with the previous API design and could break existing code that relied on the default 'BTC' value. ```suggestion def get_historical_prices(self, asset_id: str = "BTC", limit: int = 100) -> list[Price]: ```

src/app/social/reddit.py

						
				@@ -21,22 +22,20 @@ SUBREDDITS = [

				]

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 19:33:31 +02:00

Calling .list() on post.comments may load all comments into memory. Consider iterating directly over post.comments to avoid potential memory issues with large comment threads.

    for i, top_comment in enumerate(post.comments):

Calling .list() on post.comments may load all comments into memory. Consider iterating directly over post.comments to avoid potential memory issues with large comment threads. ```suggestion for i, top_comment in enumerate(post.comments): ```

src/app/utils/wrapper_handler.py

						
				@@ -48,3 +49,3 @@

				        """

				        log_info(f"{inspect.getsource(func).strip()} {inspect.getclosurevars(func).nonlocals}")

				        return self.__try_call(func, try_all=False).popitem()[1]

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 19:33:31 +02:00

The variable 'error' is initialized but may be used uninitialized if no exception occurs in the while loop before the final raise statement.

src/app/utils/wrapper_handler.py

						
				@@ -101,0 +88,4 @@

				            if not try_all:

				                log_info(f"try_call {wrapper_name}")

				            for try_count in range(1, self.retry_per_wrapper + 1):

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 19:33:31 +02:00

The variable 'error' is initialized but may be used uninitialized if no exceptions occur in the for loop before checking if results is empty.

        error = "No error captured"

The variable 'error' is initialized but may be used uninitialized if no exceptions occur in the for loop before checking if results is empty. ```suggestion error = "No error captured" ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-10-05 21:18:36 +02:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 37 out of 39 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

## Pull Request Overview Copilot reviewed 37 out of 39 changed files in this pull request and generated 1 comment. --- **Tip:** Customize your code reviews with copilot-instructions.md. <a href="/Berack96/upo-appAI/new/main/.github?filename=copilot-instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Create the file</a> or <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">learn how to get started</a>.

src/app/utils/wrapper_handler.py

						
				@@ -48,3 +49,3 @@

				        """

				        log_info(f"{inspect.getsource(func).strip()} {inspect.getclosurevars(func).nonlocals}")

				        return self.__try_call(func, try_all=False).popitem()[1]

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 21:18:36 +02:00

[nitpick] The variable 'error' is initialized but may not be used if no exceptions occur in the loop. Consider initializing it only when needed or using a more descriptive default value.

[nitpick] The variable 'error' is initialized but may not be used if no exceptions occur in the loop. Consider initializing it only when needed or using a more descriptive default value. ```suggestion ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-10-05 22:06:01 +02:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

## Pull Request Overview Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments. --- **Tip:** Customize your code reviews with copilot-instructions.md. <a href="/Berack96/upo-appAI/new/main/.github?filename=copilot-instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Create the file</a> or <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">learn how to get started</a>.

src/app/agents/models.py

						
				@@ -0,0 +87,4 @@

				        raise ValueError(f"Modello non supportato: {self}")

				    def get_agent(self, instructions: str, name: str = "", output_schema: type[BaseModel] | None = None, tools: list[Toolkit] | None = None) -> Agent:

				        """

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 22:06:00 +02:00

Using mutable default argument 'tools: list[Toolkit] = []' can lead to unexpected behavior. Use 'tools: list[Toolkit] | None = None' and handle None inside the method.

src/app/base/markets.py

						
				@@ -0,0 +1,83 @@

				from datetime import datetime

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 22:06:00 +02:00

The docstring incorrectly states 'timestamp in millisecondi' but the method accepts both milliseconds and seconds. Also, the Args section mentions 'timestamp' parameter which doesn't exist - should document 'timestamp_ms' and 'timestamp_s' parameters.

        Imposta il timestamp usando millisecondi o secondi.
        Args:
            timestamp_ms (int, opzionale): Il timestamp in millisecondi.
            timestamp_s (int, opzionale): Il timestamp in secondi.

The docstring incorrectly states 'timestamp in millisecondi' but the method accepts both milliseconds and seconds. Also, the Args section mentions 'timestamp' parameter which doesn't exist - should document 'timestamp_ms' and 'timestamp_s' parameters. ```suggestion Imposta il timestamp usando millisecondi o secondi. Args: timestamp_ms (int, opzionale): Il timestamp in millisecondi. timestamp_s (int, opzionale): Il timestamp in secondi. ```

src/app/markets/binance.py

						
				@@ -1,28 +1,31 @@

				import os

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-05 22:06:00 +02:00

The docstring states the default currency is 'USD' but then mentions it gets changed to 'USDT'. This is confusing - the documentation should clearly explain that USD gets automatically converted to USDT for Binance compatibility.

        Inizializza il wrapper di Binance con le credenziali API e la valuta di riferimento.
        Se viene fornita una valuta fiat come "USD", questa viene automaticamente convertita in una stablecoin Tether ("USDT") per compatibilità con Binance,
        poiché Binance non supporta direttamente le valute fiat per il trading di criptovalute.
        Tutti i prezzi e volumi restituiti saranno quindi denominati nella stablecoin (ad esempio, "USDT") e non nella valuta fiat originale.
        Args:
            currency (str): Valuta in cui restituire i prezzi. Se "USD" viene fornito, verrà utilizzato "USDT". Default è "USD".

The docstring states the default currency is 'USD' but then mentions it gets changed to 'USDT'. This is confusing - the documentation should clearly explain that USD gets automatically converted to USDT for Binance compatibility. ```suggestion Inizializza il wrapper di Binance con le credenziali API e la valuta di riferimento. Se viene fornita una valuta fiat come "USD", questa viene automaticamente convertita in una stablecoin Tether ("USDT") per compatibilità con Binance, poiché Binance non supporta direttamente le valute fiat per il trading di criptovalute. Tutti i prezzi e volumi restituiti saranno quindi denominati nella stablecoin (ad esempio, "USDT") e non nella valuta fiat originale. Args: currency (str): Valuta in cui restituire i prezzi. Se "USD" viene fornito, verrà utilizzato "USDT". Default è "USD". ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-10-06 11:05:14 +02:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

Copilot reviewed 40 out of 42 changed files in this pull request and generated 5 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

## Pull Request Overview Copilot reviewed 40 out of 42 changed files in this pull request and generated 5 comments. --- **Tip:** Customize your code reviews with copilot-instructions.md. <a href="/Berack96/upo-appAI/new/main/.github?filename=copilot-instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Create the file</a> or <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">learn how to get started</a>.

src/app/agents/models.py

						
				@@ -0,0 +53,4 @@

				    @staticmethod

				    def availables() -> list['AppModels']:

				        """

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-06 11:05:13 +02:00

The nested list comprehensions and multiple iterations over AppModels could be optimized by creating a set of available models first, then filtering AppModels in a single pass.

            availables_set = set(model['model'] for model in models_list['models'])
            return [model for model in AppModels if model.name.startswith("OLLAMA") and model.value in availables_set]

The nested list comprehensions and multiple iterations over AppModels could be optimized by creating a set of available models first, then filtering AppModels in a single pass. ```suggestion availables_set = set(model['model'] for model in models_list['models']) return [model for model in AppModels if model.name.startswith("OLLAMA") and model.value in availables_set] ```

src/app/base/markets.py

						
				@@ -0,0 +30,4 @@

				        """

				        Imposta il timestamp a partire da millisecondi o secondi.

				        IL timestamp viene salvato come stringa formattata 'YYYY-MM-DD HH:MM'.

				        Args:

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-06 11:05:13 +02:00

Corrected spelling of 'IL' to 'Il'.

        Il timestamp viene salvato come stringa formattata 'YYYY-MM-DD HH:MM'.

Corrected spelling of 'IL' to 'Il'. ```suggestion Il timestamp viene salvato come stringa formattata 'YYYY-MM-DD HH:MM'. ```

src/app/markets/binance.py

						
				@@ -46,31 +57,22 @@ class BinanceWrapper(BaseWrapper):

				    def get_product(self, asset_id: str) -> ProductInfo:

				        symbol = self.__format_symbol(asset_id)

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-06 11:05:13 +02:00

The automatic conversion from 'USD' to 'USDT' by appending 'T' is fragile and may not work correctly for other currencies. Consider using a proper mapping dictionary for currency conversions.

src/app/utils/wrapper_handler.py

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-06 11:05:13 +02:00

Using locals() to access 'error' variable is unreliable and may not contain the expected value. The variable should be properly tracked in the exception handling flow.

tests/utils/test_market_aggregator.py

copilot-pull-request-reviewer[bot] (Migrated from github.com) commented

2025-10-06 11:05:12 +02:00

The hour arithmetic can cause issues when the current hour is 0 or 1, as it would result in negative hours. Use timedelta for proper date arithmetic instead.

Simo93-rgb (Migrated from github.com) approved these changes 2025-10-08 16:20:58 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Berack/upo-app-agents#20

				`@@ -0,0 +1,83 @@`
				`from datetime import datetime`

				`@@ -1,28 +1,31 @@`
				`import os`