Skip to content

Provide an alternate default message factory to work with FIX50SP2 #167

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Provide an alternate default message factory to work with FIX50SP2 #167

wants to merge 1 commit into from

Conversation

david-gibbs-ig
Copy link
Contributor

This pull request provides an alternate (non-conflicting) default message factory for users of FIX50 SP2.

philipwhiuk
philipwhiuk suggested changes Dec 12, 2017
View reviewed changes
Copy link
Contributor

@philipwhiuk philipwhiuk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This seems like a lot of code duplication.

Instead, create a new constructor on DefaultMessageFactory that provides the defaultFIXTAppVerID, which you reference on line 135.

Then make the current constructor call that constructor, passing in FIX50.

When you want to default to FIX50_SP2 you can just call the new constructor with FIX50_SP2.

@david-gibbs-ig
Copy link
Contributor Author

OK I didn't want to touch the standard DefaultMessageFactory but that was to avoid merges when I thought I would be maintaining this version only in a fork. So yes.

@chrjohn
Copy link
Member

chrjohn commented Jan 25, 2018

Hi @david-gibbs-ig , thanks for the PR.
Are you able to make the changes as suggested by @philipwhiuk ?
Thanks,
Chris.

@chrjohn chrjohn added this to the QFJ 2.0.1 milestone Jan 25, 2018
@chrjohn
Copy link
Member

chrjohn commented Feb 1, 2018

Hi @david-gibbs-ig , this PR is superseded by #171 and can be closed. Do you agree?
Thanks,
Chris.

@david-gibbs-ig
Copy link
Contributor Author

@chrjohn Yes I think so thank you.

@chrjohn chrjohn removed this from the QFJ 2.0.1 milestone Feb 2, 2018
@david-gibbs-ig david-gibbs-ig deleted the fix50sp2-default-message-factory branch December 10, 2018 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@philipwhiuk philipwhiuk philipwhiuk requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@david-gibbs-ig @chrjohn @philipwhiuk