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

Order sensitive overload for compare to set #778

Merged
merged 6 commits into from May 18, 2017

Conversation

Projects
None yet
3 participants
@gasparnagy
Collaborator

gasparnagy commented Jan 31, 2017

No description provided.

@gasparnagy gasparnagy requested a review from darrencauthon Jan 31, 2017

@darrencauthon

I like the idea -- I'm just concerned about how future changes could be worked around the new boolean argument. And if we don't set it to a default value, it could force users to have to make many tedious changes to their test suites to get them to build.

@@ -25,19 +25,24 @@ private SafetyTableDiffExceptionBuilder<T> BuildTheTableDiffExceptionBuilder()
return makeSureTheFormattingWorkAboveDoesNotResultInABadException;
}
public void CompareToSet(IEnumerable<T> set)
public void CompareToSet(IEnumerable<T> set, bool sequentialEquality)

This comment has been minimized.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

I am a little concerned about exposing a boolean as a trailing argument for any method. Not just in this case, but all cases... it's a red flag. Because you know how it goes, you make this change with a boolean, then another change comes in... what do you do? Tack on more flags?

Another option might be to take in an options class, for those concerned with types, with an override that takes a dynamic object. So the syntax could be as simple as checker.CompareToSet(set, new { CheckTheSequence = true })

Allowing an optional arguments parameter will also allow us to add on existing behavior. Considering how complicated set comparisons might be, I wouldn't be surprised if people want more options for it later.

I'd at least make the boolean optional, defaulting it to the existing behavior. Just so it won't break other people's code when they upgrade.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

I am a little concerned about exposing a boolean as a trailing argument for any method. Not just in this case, but all cases... it's a red flag. Because you know how it goes, you make this change with a boolean, then another change comes in... what do you do? Tack on more flags?

Another option might be to take in an options class, for those concerned with types, with an override that takes a dynamic object. So the syntax could be as simple as checker.CompareToSet(set, new { CheckTheSequence = true })

Allowing an optional arguments parameter will also allow us to add on existing behavior. Considering how complicated set comparisons might be, I wouldn't be surprised if people want more options for it later.

I'd at least make the boolean optional, defaulting it to the existing behavior. Just so it won't break other people's code when they upgrade.

This comment has been minimized.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

By making it optional, I can also just call checker.CompareToSet(set) and I won't have to worry about breaking any test builds.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

By making it optional, I can also just call checker.CompareToSet(set) and I won't have to worry about breaking any test builds.

This comment has been minimized.

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

I agree. Actually on the "real" public API, the SetComparisonExtensionMethods.CompareToSet extension method (

public static void CompareToSet<T>(this Table table, IEnumerable<T> set, bool sequentialEquality = false)
) I have used an optional parameter. I did not think that someone would use this method directly, but you are right, we should also make this backwards compatible to be on the safe side.
Regarding the option class approach: from the trainings I do, my experience is that people are not aware of the object initializer syntax, so as long as we only have one option, I would rather choose the optional parameter, or alternatively a different API method, like CompareToSetOrderSensitive... (maybe with a better name)

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

I agree. Actually on the "real" public API, the SetComparisonExtensionMethods.CompareToSet extension method (

public static void CompareToSet<T>(this Table table, IEnumerable<T> set, bool sequentialEquality = false)
) I have used an optional parameter. I did not think that someone would use this method directly, but you are right, we should also make this backwards compatible to be on the safe side.
Regarding the option class approach: from the trainings I do, my experience is that people are not aware of the object initializer syntax, so as long as we only have one option, I would rather choose the optional parameter, or alternatively a different API method, like CompareToSetOrderSensitive... (maybe with a better name)

This comment has been minimized.

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

Oh, I like that idea! Using a a separate method, I mean... I think that would be preferable to the trailing bit parameter.

I think it might be a cultural thing, in terms of .Net versus other stacks. I've spent a lot of time in Ruby, and having the trailing "options" parameter is much more common and accepted. Nobody thinks twice about it, as the language syntax around creating a hash/dictionary is much simpler and intuitive, and nobody thinks twice about the magic-string-ness of the object... but in C# developers have a conniption fit if they don't have a type defined with parameters with XML documentation. 😄

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

Oh, I like that idea! Using a a separate method, I mean... I think that would be preferable to the trailing bit parameter.

I think it might be a cultural thing, in terms of .Net versus other stacks. I've spent a lot of time in Ruby, and having the trailing "options" parameter is much more common and accepted. Nobody thinks twice about it, as the language syntax around creating a hash/dictionary is much simpler and intuitive, and nobody thinks twice about the magic-string-ness of the object... but in C# developers have a conniption fit if they don't have a type defined with parameters with XML documentation. 😄

This comment has been minimized.

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

@darrencauthon and would would be your preferred name?

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

@darrencauthon and would would be your preferred name?

This comment has been minimized.

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

@gasparnagy Let me think on that today... perhaps my inability to think of a good name will change my mind. 😄

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

@gasparnagy Let me think on that today... perhaps my inability to think of a good name will change my mind. 😄

This comment has been minimized.

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

@darrencauthon in Cucumber JVM the two method is called diff and unorderedDiff BTW. So it seems that also decided to go for two separate methods... https://cucumber.github.io/api/cucumber/jvm/javadoc/cucumber/api/DataTable.html

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

@darrencauthon in Cucumber JVM the two method is called diff and unorderedDiff BTW. So it seems that also decided to go for two separate methods... https://cucumber.github.io/api/cucumber/jvm/javadoc/cucumber/api/DataTable.html

@@ -8,7 +8,7 @@ public class SetComparer<T>
{
private const int MatchNotFound = -1;
private readonly Table table;
private List<T> actualItems;
private List<TableDifferenceItem<T>> extraOrNonMatchingActualItems;

This comment has been minimized.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

I saw this pull request earlier, and I swear I saw you using a Tuple? I thought that was interesting, as I'm finding tuples more and more useful for internal behaviors in my own code.

@darrencauthon

darrencauthon Feb 2, 2017

Contributor

I saw this pull request earlier, and I swear I saw you using a Tuple? I thought that was interesting, as I'm finding tuples more and more useful for internal behaviors in my own code.

This comment has been minimized.

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

Yes. It was this commit: 2796780
I was also wanted to see how Tuples could be better used, but altogether I did not really like the result. It was very hard to communicate with the code what data I am combining together (in my case the item with the expected position). So I refactored it.

@gasparnagy

gasparnagy Feb 6, 2017

Collaborator

Yes. It was this commit: 2796780
I was also wanted to see how Tuples could be better used, but altogether I did not really like the result. It was very hard to communicate with the code what data I am combining together (in my case the item with the expected position). So I refactored it.

This comment has been minimized.

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

Yeah, my tuples regularly get refactored to types as well. It's nice to have them as an option, though, because in the middle of working I don't always want to load my brain with more types. I'd rather establish the flow of the code and then refactor it afterwards.

I think the tuples make more sense in other languages that make unpacking them simpler.

@darrencauthon

darrencauthon Feb 6, 2017

Contributor

Yeah, my tuples regularly get refactored to types as well. It's nice to have them as an option, though, because in the middle of working I don't always want to load my brain with more types. I'd rather establish the flow of the code and then refactor it afterwards.

I think the tuples make more sense in other languages that make unpacking them simpler.

@gasparnagy gasparnagy changed the title from WIP: order sensitive overload for compare to set to Order sensitive overload for compare to set May 18, 2017

@gasparnagy

This comment has been minimized.

Show comment
Hide comment
@gasparnagy

gasparnagy May 18, 2017

Collaborator

@darrencauthon we will merge this with @SabotageAndi now so that it can be included in the pre-relase today.
We can try it out live with the pre-release and fix the naming still.

Collaborator

gasparnagy commented May 18, 2017

@darrencauthon we will merge this with @SabotageAndi now so that it can be included in the pre-relase today.
We can try it out live with the pre-release and fix the naming still.

@SabotageAndi SabotageAndi merged commit 0d7e80f into master May 18, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Autom8edChaos added a commit to Autom8edChaos/SpecFlow that referenced this pull request Nov 6, 2017

Update from original (#1)
* update MSBuild.Community.Tasks to latest version via NuGet (#801)

* Regexless stepdef fixes (#786)

* Upgrade FluentAssertions

* Add simple unit test

* Add unit test for parametrized method

* Add unit tests for regexless - 36 failing

* fluent assertion does not allow { and } in the because part

* refine expectations: shortening like "doesn't" should not work automatically

* fix regexless matching issues

* a bit more optimal solution that produces simpler regexes

* Improve param locator

* should not remove keywords from binding culture

* small code improvements

* refine expectations: should not allow whitespace or punctuation in the front of the step text (Given !foo) in order to make the generated regex faster (does not start with rule)

* small refactor

* update changelog, remove wip tags

* add test for issue #715

* Update changelog showing that #301 is also fixed

* add CI NuGet package feed

* Added support for MsTest's [DeploymentItem] attribute with @mstest:DeploymentItem tag (#805)

* Added support for MsTest's [DeploymentItem] attribute with @deploy tag (Issue #803)
* Prefixing new tag with `MsTest:`
* using pascal case @mstest:DeploymentItem
* wrapping nested using blocks with curly brackets

* Change project reader from MSBuild to XML (#837)

* improve tests for project reader
* add tests for new csproj format
* change variable naming
* complete read of classic projects
* refactoring
* mark not used properties as obsolete
* support new project system & refactorings
* use AppDomainIsolatedTask instead of Task to solve file looking issues with MsBuild
* fix typo :-/
* add tests for linked files

* Added display name for theory attribution (#812)

* Added display name for theory attribution
* Fixed incorrect namespace reference to TheoryAttribute (no longer part of extensions)

* Support context parameters on Before/After methods, FeatureContainer (#779)

* Added scenario: Inject FeatureContext into a BeforeFeature hook
* first dummy implementation to make the scenario pass (runtime tests failing)
* Use bindingInstanceResolver to resolve parameters
* make unit tests pass
* Add unit tests for what we have so far
* support for resolving hook parameters from scenario context
* better then step for the scenarios
* fix unit test error
* added failing test for resolving objects from feature container
* refactor ScenarioContext
* Implement FeatureContainer
* move binding culture initialization to FeatureContext
* fix featurecontext resolution from scenario container
* Refactor scenario and feature context and remove displose hack that was necessary to avoid circular disposing loops
* fix unit test failure
* Make InternalContextManager to IObjectContainer reference more explicit
* rename IBindingInstanceResolver to ITestObjectResolver (breaking change for some plugins)

* Wildcard support for project reader (#843)

* Wildcard support for project reader
* fix tests for AppVeyor

* update changelog

* Config file refactoring & Json Config file support (#690)

* split app.config elements to multiple files

* make RuntimeConfiguration nearly immutable

* extract config loading logic to RuntimeConfigurationLoader

* reformating

* add tests for app.config reading

* move ConfigurationTest to separate Namespace

* rename file

* add json tests

* add json.net dependency

* json config file parsing

* add tests for checking which config file is used

* refactoring for tests and added tests

* remove GeneratorConfiguration

* fix dependencies for generator

* fix tests

* fix tests

* move files & fix namespaces

* work on generation config loading

* fix tests and make parsing more robust

* fix and comment test

* small cleanups

* remove ToolLanguage, GeneratorPath and DetectAmbiguousMatches from Config

rename PrintConfigSource to TraceConfigSource

* fix merge issues

* remove file

* add missing json handling

* update changelog

* add Newtonsoft.Json dependency to the NuGet package

* update TestGenerator version

* fix typo in README.md (#853)

* fix VS code behind generation (#855)

* fix redirects
* fix serialization problem with Visual Studio
* fix lineending

* Order sensitive overload for compare to set (#778)

* Add tests
* Refactor TableDifferenceResults to be able to represent order sensitive diff
* make throw tests pass
* refactor message tests to be able to test order sensitive comparison
* fix diff messages for order sensitive
* Replace Tuple with TableDifferenceItem

* Hook with multiple Tags scoped are executed more than once (#848)

* add failing tests
* Fix for #848 Hook with multiple Tags scoped are executed more than once

* making sure that the ordering is preserved (the GroupBy might change the order)

* update changelog for 2.2.0-prerelease20170523

* add GitExtensions configuration file with github and appveyor configuration (#862)

* Added TestThreadContext and exposed it in ContextManager and Steps base class (#875)

* Added TestThreadContext and exposed it in ContextManager and Steps base class

* Upgrade to BoDi v1.3 (#876)

* fix shadow copy test issue
* upgrade BoDi to v1.3
* remove unnecessary BoDi workarounds

* Adds support for xUnit2 ITestOutputHelper #575 (#874)

* Resolves #575 

* added tests for adding support for xUnit2 ITestOutputHelper class

added XUnit2Generator unit tests
added XUnit2Provider specs
updated XUnitExecutionDriver to output results in default (xUnit) xml
output instead of NUnit xml format

* swapped the order of the TestInitialize call and the setting of the _testOutputHelper field

* expose TestAssembly and BindingAssemblies on ITestRunnerManager

* Fix generation from visual studio (#877)

* rename field so that it matches the version of this class from 1.9 (which is used by VS)
* make AppConfig the default value
* add warnings

* Update for 2.2 release

* use FileStream constructor which grants ReadWrite to other processes. (#906)

use FileStream constructor which grants ReadWrite to other processes

techtalk#904

* Added changelog entry for PR (#907)

* correct version number

* Access MSTest's TestContext property via ScenarioContext (#882)

* injected MSTest's TestContext into ScenarioContext

* fixed TestContext property generation for VB.Net

* @MSBuild [DeploymentItem] - added option to provide output directory (#901)

* @MSBuild [DeploymentItem] - added option to provide output directory

* code review

* update changelog after merging PRs #882 & #901

* add tests for Feature/Scenario Hooks with context parameters (#925)

* Issue Template (#924)

* Create issue template

* add SpecFlow+Runner to test runners

* Rename issue to ISSUE_TEMPLATE.md

* Allowed custom XSLT scripts to contain scripts.

* Updated changelog for new addition

* enter release date

* fix changelog after 2.2.1 release

* added link to Stack Overflow repro topic (#942)

* Avoid trying to load empty configuration

* Set ConfigSource of default holder to "Default" instead of "AppConfig"

* Turn off line ending git auto conversion (#953)

* Stop git from automatically converting line endings everytime repo is cloned
* add .vs folder to .gitignore

* Fixing copy link in step definition report (#958)

* Update changelog

* Support for tuples (#951)

* Initial support for Tuples
* Adding nuspec dependency
* Added tupple error when there are too many properties

* add entry for tuple

* Make scenario TestResult public (#963)

* Make scenario TestResult public

* - Rename to ScenarioExecutionStatus
- Moved this to the root of the project
- UndefinedStep in enum
- Edited changelog.txt

* Update changelog.txt

* Single agent for unit tests execution

* Fix incorrect appveyour config

* Try to change test_script to test

* Disable automatic tests

* Provide full path to test assemblies

* Expose configuration var to batch

* Hardcoded configuration as Release

* Use %% accessor for configuration var

* Add hyperlink to discussion group (#965)

Remove ambiguity regarding where to go for questions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment