Skip to content

Conversation

Arthurm1
Copy link
Contributor

This is a weekday based session schedule that can be used instead of DefaultSessionSchedule.

It uses a new session setting ID Weekdays which is a comma-delimited list of the days to start the session on.

A standard western trading week example...

StartTime=22:05:00 UTC
EndTime=21:55:00 UTC
Weekdays=Sun,Mon,Tue,Wed,Thu

StartDay and EndDay are not used.

To tell QuickFIX/J to use the weekly schedule, a WeekdaySessionScheduleFactory instance must be passed to the DefaultSessionFactory constructor.

@philipwhiuk
Copy link
Contributor

Hmm..

Did you consider just adding the logic to the DefaultSessionSchedule? It feels like this is a comparable behavioural change to Non-Stop-Session or Weekly Sessions to me. Why not just have it be used if Weekdays is set, rather than having to pick a different SessionSchedule and then that Schedule throw an error when it's not set?

By making it a factory choice you have to pick for an entire QFJ session factory which will be used, rather than picking on a session by session basis.

@Arthurm1
Copy link
Contributor Author

When I wrote this it was outside the QuickFIXJ codebase so I didn't have the option to refactor DefaultSessionSchedule.

You're probably right though. I could have a look at integrating it into DefaultSessionSchedule if people prefer that and it's likely to be accepted.

@chrjohn chrjohn added this to the QFJ 1.7.0 milestone Aug 25, 2017
Copy link
Member

@chrjohn chrjohn left a comment

Choose a reason for hiding this comment

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

Hi @Arthurm1 , thanks for the PR. As @philipwhiuk pointed out it would be better to have this on a per-Session basis.
But I see nothing against merging this and we can do the changes later as it is an improvement to the current situation.
However, could you please also add the parameter to the configuration.html file?
Thanks,
Chris.

@chrjohn
Copy link
Member

chrjohn commented Aug 25, 2017

Hi @Arthurm1 , I just started to work on unifying the WeekdaySessionSchedule and DefaultSessionSchedule. So if you could just add the missing documentation that would be great.
Thanks,
Chris.

@Arthurm1
Copy link
Contributor Author

@chrjohn That's great. I was going to take a look at it myself next week but if you've already started.

I'm good to add Weekdays into the config docs.

Looking at how StartDay, EndDay are translated using DayConverter I think I've put too much validation into the WeeklySessionSchedule. It will reject any days that are not 3 letters (e.g. "Mon") but I don't think that is consistent with how StartDay, EndDay work and may not cope with non-English days so you could probably just take that out.

@chrjohn chrjohn merged commit 39183ce into quickfix-j:master Aug 25, 2017
chrjohn added a commit that referenced this pull request Aug 25, 2017
@chrjohn
Copy link
Member

chrjohn commented Aug 25, 2017

Hi @Arthurm1 , see #134 :)
Thanks again for the PR!

chrjohn added a commit that referenced this pull request Aug 25, 2017
Integrated changes done by Arthur McGibbon @Arthurm1 in #133 into DefaultSessionSchedule
@Arthurm1
Copy link
Contributor Author

Thanks for putting it in the DefaultSessionSchedule - that looks better than my extra classes.

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