Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: defined a type for exchange kinds #286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

techerfan
Copy link

The developer must type out the kind of exchange when declaring it, which could lead to typos. Also, the IDE or code editor did not help or suggest a type of exchange.
This way, selecting an exchange is more straightforward and helps maintain the code more easily.

@lukebakken lukebakken self-assigned this Nov 4, 2024
@lukebakken lukebakken added this to the 2.0.0 milestone Nov 4, 2024
@Zerpet
Copy link
Contributor

Zerpet commented Nov 4, 2024

I agree with the idea, but the current implementation changes the public API, and it's a breaking change. We can merge this PR when we plan to release 2.0.

In the mean time, you can accomplish the same without declaring the ExchangeType, simply declaring the string constants. If you can submit a different PR (keep this one open please), I'll gladly merge a non-breaking change.

@techerfan
Copy link
Author

It's ok. I'll keep the PR open.

In the mean time, you can accomplish the same without declaring the ExchangeType, simply declaring the string constants. If you can submit a different PR (keep this one open please), I'll gladly merge a non-breaking change.

These string constants were defined before. I just changed their name and added ExchangeType to them.

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