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

Improve value retriever/comparer registration #1260

Merged

Conversation

szaliszali
Copy link
Contributor

Fixes #1218

I think there is too much similarity in comparer and retriever handling in the Assist service, so I propose they could be merged into one with a small API change:

Before:

Service.Instance.RegisterValueComparer(___)

After:

Service.Instance.ValueComparers.Register(___)

Additions:

Service.Instance.ValueComparers.Register<MyClass>()
Service.Instance.ValueComparers.Unregister<MyClass>()
Service.Instance.ValueComparers.Replace(oldObj, newObj)
Service.Instance.ValueComparers.Replace<OldClass, NewClass>()

Pros:

  • simplifies future maintenance

Cons:

  • breaking change
  • RegisterDefault only applied to comparers in the past (and it's weird to add more "default"s to a single list but I did not change this behavior)

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added an entry to the changelog

@kevinkuszyk
Copy link
Contributor

Posting a note here to highlight #1267 also has a fix for #1218 (I had already written the code before I saw this PR).

Only one of these two PRs should be merged.

Copy link
Contributor

@SabotageAndi SabotageAndi left a comment

Choose a reason for hiding this comment

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

I like the idea with the ServiceComponentList class, so I think I prefer this PR over #1267.
@szaliszali Could you add an entry in the changelog for this.

@SabotageAndi SabotageAndi added this to the SpecFlow 3.0 milestone Sep 19, 2018
@szaliszali szaliszali force-pushed the 1218-improved-retriever-registration branch from 9e8025d to 11a68f2 Compare September 19, 2018 11:25
@szaliszali szaliszali force-pushed the 1218-improved-retriever-registration branch from f53ffbd to 65bd6ec Compare September 19, 2018 18:01
@szaliszali
Copy link
Contributor Author

rebased the branch on current master and force pushed to solve merge conflict without the mess

@SabotageAndi SabotageAndi merged commit 9d6a4ae into SpecFlowOSS:master Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some syntactic sugar to make registering and un-registering value retrievers easier?
3 participants