12 fix docs #13

2025-10-02T01:24:20+02:00

Berack96 commented

2025-10-02 01:24:20 +02:00

(Migrated from github.com)

Come scritto nei commit, ho fatto un pò di pulizia del codice e di chiarezza.
Alcune parti erano da aggiustare (come le chiavi e le env) e quindi le ho messe a posto.
Altre parti ho fatto un refactor, come per esempio l'aggregator (dato che era un pò complesso per quello che faceva) e quindi l'ho ridotto ad una funzione che aggrega i dati che gli arrivano.
Inoltre ho anche aggiunto la funzione di aggregazione delle candele (history)

Gli assert sono stati lasciati come utilizzo di check per i valori. Anche se in produzione è meglio avere un ValueError è più comodo avere degli assert per chiarezza del codice.
La parte di inspect.getsource() è lasciata per generare un log migliore quando vengono usate le API

Come scritto nei commit, ho fatto un pò di pulizia del codice e di chiarezza. Alcune parti erano da aggiustare (come le chiavi e le env) e quindi le ho messe a posto. Altre parti ho fatto un refactor, come per esempio l'aggregator (dato che era un pò complesso per quello che faceva) e quindi l'ho ridotto ad una funzione che aggrega i dati che gli arrivano. Inoltre ho anche aggiunto la funzione di aggregazione delle candele (history) Gli assert sono stati lasciati come utilizzo di check per i valori. Anche se in produzione è meglio avere un ValueError è più comodo avere degli assert per chiarezza del codice. La parte di `inspect.getsource()` è lasciata per generare un log migliore quando vengono usate le API

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

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

Pull Request Overview

This PR implements code cleanup and refactoring with the following changes:

Refactored market data aggregation from a complex class-based system to simple utility functions
Updated all timestamp fields from time strings to timestamp_ms integers for consistency
Removed deprecated methods and classes to simplify the codebase
Enhanced test coverage with additional test cases for wrapper handling
Improved error handling and logging throughout the application

Reviewed Changes

Copilot reviewed 37 out of 40 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/utils/test_wrapper_handler.py	Added new test cases for wrapper handling with parameters
tests/utils/test_market_data_aggregator.py	Removed complex aggregator tests
tests/utils/test_market_aggregator.py	Added simplified aggregation function tests
tests/tools/test_news_tool.py	Removed `@pytest.mark.limited` marker
tests/tools/test_market_tool.py	Cleaned up imports and removed unused test methods
tests/conftest.py	Reorganized test markers for better structure
tests/api/*.py	Updated test assertions to use `timestamp_ms` instead of `time`
src/app/utils/wrapper_handler.py	Enhanced logging and error handling
src/app/utils/market_data_aggregator.py	Removed complex aggregator class
src/app/utils/market_aggregation.py	Added simplified aggregation functions
src/app/utils/aggregated_models.py	Removed complex aggregated models
src/app/markets/*.py	Updated to use `timestamp_ms` and removed deprecated methods
src/app/news/init.py	Added aggregated news retrieval methods
src/app/pipeline.py	Simplified agent initialization and removed imports
src/app/social/reddit.py	Improved assertion messages
pyproject.toml	Added blank line for formatting
docker-compose.yaml	Simplified environment configuration
README.md	Major documentation update with clearer installation instructions

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

## Pull Request Overview This PR implements code cleanup and refactoring with the following changes: - Refactored market data aggregation from a complex class-based system to simple utility functions - Updated all timestamp fields from `time` strings to `timestamp_ms` integers for consistency - Removed deprecated methods and classes to simplify the codebase - Enhanced test coverage with additional test cases for wrapper handling - Improved error handling and logging throughout the application ### Reviewed Changes Copilot reviewed 37 out of 40 changed files in this pull request and generated 6 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | tests/utils/test_wrapper_handler.py | Added new test cases for wrapper handling with parameters | | tests/utils/test_market_data_aggregator.py | Removed complex aggregator tests | | tests/utils/test_market_aggregator.py | Added simplified aggregation function tests | | tests/tools/test_news_tool.py | Removed `@pytest.mark.limited` marker | | tests/tools/test_market_tool.py | Cleaned up imports and removed unused test methods | | tests/conftest.py | Reorganized test markers for better structure | | tests/api/*.py | Updated test assertions to use `timestamp_ms` instead of `time` | | src/app/utils/wrapper_handler.py | Enhanced logging and error handling | | src/app/utils/market_data_aggregator.py | Removed complex aggregator class | | src/app/utils/market_aggregation.py | Added simplified aggregation functions | | src/app/utils/aggregated_models.py | Removed complex aggregated models | | src/app/markets/*.py | Updated to use `timestamp_ms` and removed deprecated methods | | src/app/news/__init__.py | Added aggregated news retrieval methods | | src/app/pipeline.py | Simplified agent initialization and removed imports | | src/app/social/reddit.py | Improved assertion messages | | pyproject.toml | Added blank line for formatting | | docker-compose.yaml | Simplified environment configuration | | README.md | Major documentation update with clearer installation instructions | </details> --- <sub>**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>.</sub>

src/app/markets/cryptocompare.py

						
				@@ -23,1 +17,3 @@

				    price.time = str(price_data['time'])

				    price.high = float(price_data.get('high', 0))

				    price.low = float(price_data.get('low', 0))

				    price.open = float(price_data.get('open', 0))

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

2025-10-02 01:29:52 +02:00

Using assertions for API validation is not recommended in production code. Consider raising a more specific exception like ValueError or APIDataError instead.

    if product.price <= 0:
        raise ValueError("Invalid price data received from CryptoCompare")

Using assertions for API validation is not recommended in production code. Consider raising a more specific exception like `ValueError` or `APIDataError` instead. ```suggestion if product.price <= 0: raise ValueError("Invalid price data received from CryptoCompare") ```

src/app/markets/cryptocompare.py

						
				@@ -35,3 +35,3 @@

				    def __init__(self, currency:str='USD'):

				        api_key = os.getenv("CRYPTOCOMPARE_API_KEY")

				        assert api_key is not None, "API key is required"

				        assert api_key, "CRYPTOCOMPARE_API_KEY environment variable not set"

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

2025-10-02 01:29:52 +02:00

Using assertions for API validation is not recommended in production code. Consider raising a more specific exception like ValueError or APIDataError instead.

    if price.timestamp_ms <= 0:
        raise ValueError("Invalid timestamp data received from CryptoCompare")

Using assertions for API validation is not recommended in production code. Consider raising a more specific exception like `ValueError` or `APIDataError` instead. ```suggestion if price.timestamp_ms <= 0: raise ValueError("Invalid timestamp data received from CryptoCompare") ```

src/app/markets/yfinance.py

						
				@@ -3,63 +3,29 @@ from agno.tools.yfinance import YFinanceTools

				from .base import BaseWrapper, ProductInfo, Price

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

2025-10-02 01:29:51 +02:00

The comment says the currency is after the '-' but the code takes the part before it (index 0). This logic appears incorrect - for 'BTC-USD', this would set quote_currency to 'BTC' instead of 'USD'.

    product.quote_currency = product.id.split('-')[1] if '-' in product.id else ''  # La valuta è la parte dopo il '-'

The comment says the currency is after the '-' but the code takes the part before it (index 0). This logic appears incorrect - for 'BTC-USD', this would set quote_currency to 'BTC' instead of 'USD'. ```suggestion product.quote_currency = product.id.split('-')[1] if '-' in product.id else '' # La valuta è la parte dopo il '-' ```

src/app/utils/wrapper_handler.py

						
				@@ -48,2 +51,4 @@

				        iterations = 0

				        while iterations < len(self.wrappers):

				            wrapper = self.wrappers[self.index]

				            wrapper_name = wrapper.__class__.__name__

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

2025-10-02 01:29:51 +02:00

Using inspect.getsource() in production code can be expensive and may fail in some environments (compiled code, frozen executables). Consider logging only essential information like function name.

        log_info(f"Calling function '{func.__name__}' from module '{func.__module__}'")

Using `inspect.getsource()` in production code can be expensive and may fail in some environments (compiled code, frozen executables). Consider logging only essential information like function name. ```suggestion log_info(f"Calling function '{func.__name__}' from module '{func.__module__}'") ```

src/app/utils/wrapper_handler.py

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

2025-10-02 01:29:51 +02:00

Using inspect.getsource() in production code can be expensive and may fail in some environments (compiled code, frozen executables). Consider logging only essential information like function name.

        log_info(f"Calling function '{func.__name__}' from module '{func.__module__}' with nonlocals: {inspect.getclosurevars(func).nonlocals}")

Using `inspect.getsource()` in production code can be expensive and may fail in some environments (compiled code, frozen executables). Consider logging only essential information like function name. ```suggestion log_info(f"Calling function '{func.__name__}' from module '{func.__module__}' with nonlocals: {inspect.getclosurevars(func).nonlocals}") ```

tests/utils/test_market_aggregator.py

						
				@@ -0,0 +1,120 @@

				import pytest

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

2025-10-02 01:29:50 +02:00

Debug print statement should be removed from production test code. Use logging or remove this line entirely.

Debug print statement should be removed from production test code. Use logging or remove this line entirely. ```suggestion ```

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

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

Pull Request Overview

Copilot reviewed 38 out of 41 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 38 out of 41 changed files in this pull request and generated 3 comments. --- <sub>**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>.</sub>

src/app/markets/yfinance.py

						
				@@ -5,3 +5,3 @@

				def create_product_info(symbol: str, stock_data: dict) -> ProductInfo:

				def create_product_info(stock_data: dict[str, str]) -> ProductInfo:

				    """

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

2025-10-02 01:37:57 +02:00

The type annotation dict[str, str] is too restrictive. The function expects numeric values for price calculations but the annotation suggests all values are strings, which could lead to type errors during float conversion.

src/app/utils/market_aggregation.py

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

				    if unknown_sources > 0:

				        score *= (1 - unknown_sources / len(sources))

				    return max(0.0, min(1.0, score))

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

2025-10-02 01:37:58 +02:00

The _calculate_confidence function is defined but never used in the codebase. Consider removing this unused function or documenting its intended future use.

The _calculate_confidence function is defined but never used in the codebase. Consider removing this unused function or documenting its intended future use. ```suggestion ```

src/app/utils/wrapper_handler.py

						
				@@ -48,2 +51,4 @@

				        iterations = 0

				        while iterations < len(self.wrappers):

				            wrapper = self.wrappers[self.index]

				            wrapper_name = wrapper.__class__.__name__

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

2025-10-02 01:37:57 +02:00

Using inspect.getsource() for logging may have performance implications as it reads and parses source code on every function call. Consider adding a debug flag to conditionally enable this detailed logging.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Berack/upo-app-agents#13

				`@@ -3,63 +3,29 @@ from agno.tools.yfinance import YFinanceTools`
				`from .base import BaseWrapper, ProductInfo, Price`

				`@@ -0,0 +1,120 @@`
				`import pytest`