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

Added Pick functionality to builders #49

Merged
merged 4 commits into from
Feb 7, 2016
Merged

Added Pick functionality to builders #49

merged 4 commits into from
Feb 7, 2016

Conversation

mwhelan
Copy link
Member

@mwhelan mwhelan commented Jan 19, 2016

Ability to pick items from a collection during building. Uses the existing generators to either select items at random or in a repeatable sequence.

@robdmoore
Copy link
Member

This looks really clean. If you are happy the data sources stuff can't be reused I'm happy to merge.

@mwhelan
Copy link
Member Author

mwhelan commented Jan 21, 2016

I refactored it to use DataSources. What do you think @robdmoore ? Go ahead and commit if you like it, otherwise I'm happy to make any changes.

protected override IList<T> InitializeDataSource()
{
// This method will never be called as the list is set in the constructor.
throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively the constructor could store the list in a private field and this method could return the lost? Then you might not need the internal set on the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is there is already a _list field. I don't really like the idea of a second field to store the same thing.

One option is to make the field protected, in which case it becomes available to this constructor and I can just set it directly. The reason I didn't go for that though is it then becomes accessible to every class that inherits from it, something I didn't really want to do. Making the Setter internal achieves what I want, because I can set it from this class but consumers don't gain access to the private field.

I could return new List() from the InitializeDataSource method, but I thought that was misleading as it implied it might get called. This method is only called in the lazy instantiation of the list, meaning it will never be called in this implementation as we are setting the list in the constructor. If someone were to inherit from this class and call this method I'm happy it would give them an exception. They can just override it in their derived class with the behaviour they expect.

That was my thinking anyway. Let me know if you disagree at all. :-)

@mwhelan
Copy link
Member Author

mwhelan commented Feb 4, 2016

(Partly?) addresses #27

@robdmoore
Copy link
Member

Do you mind submitting a PR to the readme to add documentation for this new feature?

Also, we should bump the minor version in https://github.com/TestStack/TestStack.Dossier/blob/master/GitVersionConfig.yaml. I'll do that manually after merging this though.

robdmoore added a commit that referenced this pull request Feb 7, 2016
Added Pick functionality to builders
@robdmoore robdmoore merged commit 365cd06 into TestStack:master Feb 7, 2016
@mwhelan
Copy link
Member Author

mwhelan commented Feb 7, 2016

Yes, happy to submit a PR to document the feature.

@mwhelan
Copy link
Member Author

mwhelan commented Feb 10, 2016

Picking documentation here: http://dossier.teststack.net/v1.0/docs/picking-list-items

@mwhelan mwhelan deleted the picking branch February 10, 2016 09:45
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