Skip to content

Made story title prefix customisable.#106

Merged
MehdiK merged 2 commits intoTestStack:masterfrom
mwhelan:master
Apr 10, 2014
Merged

Made story title prefix customisable.#106
MehdiK merged 2 commits intoTestStack:masterfrom
mwhelan:master

Conversation

@mwhelan
Copy link
Copy Markdown
Member

@mwhelan mwhelan commented Apr 10, 2014

Implements #68. Allows you to customise the story title prefix for all reports by setting TitlePrefix property in the Story attribute. Defaults to current behaviour of "Story: "
Implements #95. Added RunDate to FileReportModel and removed date funcs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not default titlePrefix to "Story: " here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did initially do that. However, I realised that when the other constructor was called, it passes in narrative.TitlePrefix to that parameter and would override that default value. I would expect narrative.TitlePrefix to often be null. That would mean I would need to set a default for that constructor too:

narrative.TitlePrefix ?? "Story: "

which means I'm effectively setting the default twice. Doing it in the constructor means I only have to set the default value once. And it's kinda consistent with how Title is being set.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool. Thanks :)

@MehdiK
Copy link
Copy Markdown
Member

MehdiK commented Apr 10, 2014

This is a small but very useful improvement. I like it :)

There is only one small nitpicking that would be good if you could fix. Will merge then.

MehdiK added a commit that referenced this pull request Apr 10, 2014
Made story title prefix customisable.
@MehdiK MehdiK merged commit bd45374 into TestStack:master Apr 10, 2014
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