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

Add order_class to the parsed order response. #28

Closed
wants to merge 1 commit into from
Closed

Add order_class to the parsed order response. #28

wants to merge 1 commit into from

Conversation

husafan
Copy link

@husafan husafan commented Apr 29, 2022

@d-e-s-o d-e-s-o self-requested a review April 30, 2022 04:18
@d-e-s-o d-e-s-o self-assigned this Apr 30, 2022
Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Thanks! Will merge after the markets open next.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 30, 2022

Seems fine to me. Thanks! Will merge after the markets open next.

Actually, nevermind; I merged it now.

@d-e-s-o d-e-s-o closed this Apr 30, 2022
@husafan husafan deleted the order_class branch April 30, 2022 19:08
@husafan
Copy link
Author

husafan commented Apr 30, 2022

Thank you!

What is the typical process for releasing the crate and how long does that take? I was hoping to use this change from apcacli to print the order type in the CLI output. No rush, just curious.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 30, 2022

What is the typical process for releasing the crate and how long does that take? I was hoping to use this change from apcacli to print the order type in the CLI output. No rush, just curious.

There is no fixed timeline. Once the subjective measure of "sufficient changes to justify a release" has been reached it will happen. The barrier for patch releases (compatibility preserving) is typically much lower. Right now this is the only change we have for the next release. What's worse, this change is compatibility breaking. We can accelerate if there is a need, though. It's just a hassle for everyone if every commit were to create a new minor release that requires a bump in downstream code.

@husafan
Copy link
Author

husafan commented Apr 30, 2022

Nope, no need to accelerate anything. And thank you for taking the time to explain. I have both branches locally and can use my versions for development and testing. When I see the new version, I'll send another pull request for apcacli. In the meantime, are there any issues you want help with on apca?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 30, 2022

In the meantime, are there any issues you want help with on apca?

Some ideas that I have on my TODO list are the following:

Since you were also looking into apcacli:

  • don't use colors when input is not a TTY
  • format decimal values (and prices) as per the current locale

Some of them may be significantly more involved than others; I've ordered them roughly by increasing anticipated complexity. Some are mostly investigative at this point and further work will depend on the outcome of the investigation. Happy to provide more context if you feel that something piques your interest, in which case I'd suggest you open a quick issue to discuss some more.

@husafan
Copy link
Author

husafan commented Apr 30, 2022

This is extremely helpful. I will work on these with you. Thanks!

@d-e-s-o
Copy link
Owner

d-e-s-o commented May 7, 2022

Hm, only noticing now that this member should have been called class instead of order_class :-| Will fix it up. Luckily we haven't cut a release yet.

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.

2 participants