Skip to content

Conversation

@GuillaumeDSM
Copy link
Member

@GuillaumeDSM GuillaumeDSM requested a review from Herklos November 26, 2025 10:03
@GuillaumeDSM GuillaumeDSM self-assigned this Nov 26, 2025
"""
try:
await client.load_markets(reload=reload)
if constants.FETCH_MIN_EXCHANGE_MARKETS and market_filter:
Copy link
Member

Choose a reason for hiding this comment

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

Why a constant and not a connector class attribute?

Copy link
Member Author

@GuillaumeDSM GuillaumeDSM Nov 26, 2025

Choose a reason for hiding this comment

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

we don't always want to do this. this filter is very strict (it prevents CCXT from even considering certain markets, unlike regular market filters which allow ccxt to load all markets and then filters out unrequired ones in next created exchanges). It should only be used in specific contexts, it's not a "per exchange" thing.
That's why it's a constant bound to an env var: it can be enabled but only if the user explicitely asks for it (it uses ccxt common interface so it should work for all ccxt exchanges)

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why an environment variable vs a parameter that could be based on a env var value. I don't get why we should use an env var here. That's my point. If this value is based on an environment var somewhere else why not. I don't understand why we want to "lock" this value with an environment variable instead of a class param that could use the value of an environment variable

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, it's up!

@GuillaumeDSM GuillaumeDSM enabled auto-merge (rebase) November 26, 2025 12:41
@GuillaumeDSM GuillaumeDSM merged commit 15333b9 into master Nov 26, 2025
5 checks passed
@GuillaumeDSM GuillaumeDSM deleted the mini branch November 26, 2025 12:46
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.

3 participants