-
Notifications
You must be signed in to change notification settings - Fork 618
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 extra options to session class #236
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @philipwhiuk ,
thanks for the PR. I have some questions:
Fix41ResendRequestAsFix42
Is it sensible to change this behaviour only for the receiving side? Don't you want to send ResendRequest for FIX4.1 with infinity conveyed as 0 also?
In general, how do do you feel about renaming this option? Maybe something along the lines of ResendUsesZeroAsInfinity or something like that? On FIX4.2+ this option would of course be enabled implicitly.
I also do not know if it makes sense to make this a FIX4.1 only feature. The same could apply to FIX4.0.
Still thinking about the PossDup stuff but sounds useful.
Cheers,
Chris.
I'll have to work out how to handle it if we rename them. We can probably deprecate and map the old values in our code before passing them to QFJ. I'm open to renaming it and making it more general. In terms of sending that does make sense. I'll see if we have any active usage of it these days (it's been internal for way way too long) and see whether we can try to see whether it was intentionally only done on one side. I'll try to write some more tests for both pieces before we merge this in :) |
Hi @chrjohn, What needs doing on this PR? I'm happy to help moving it forward Thanks |
Hi @esanchezros. I'll update this tomorrow with some tests and the changes from Chris and then hopefully it can be merged :) |
Hi @philipwhiuk, |
personally I would prefer to have each feature in a single PR, so if it is OK with you (@philipwhiuk) then I would be happy if we could proceed with #395 for the PossDup thing and leave this PR for the FIX4.1/4.2 Resend option. What do you think? |
We have long had some internal settings that I feel like upstream will benefit from (also it makes my life easier).
AllowPosDup
PossDupFlag
is set when resending messages. Currently it's automatically removed by QFJ.This PR makes two changes:
Fix41ResendRequestAsFix42