Skip to content
This repository was archived by the owner on Feb 3, 2026. It is now read-only.

add full requirements - #1300

Merged
GuillaumeDSM merged 5 commits into
masterfrom
mini
Nov 26, 2025
Merged

GuillaumeDSM merged 5 commits into
masterfrom
mini

Conversation

@GuillaumeDSM

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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?

@GuillaumeDSM GuillaumeDSM Nov 26, 2025 •

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants