Ensure lack of optional parameters #1031

nulltoken · 2015-04-25T18:55:22Z

Create a MetaFixture ensuring no optional parameter is used
See the test fail
🔥 the optionals
See the test pass

shiftkey · 2015-04-25T18:59:12Z

😍 😍 😍 😍 😍

@nulltoken did you want an extra pair of hands to help with cleaning this up? otherwise I'm happy to help with reviewing...

shiftkey · 2015-04-25T19:00:06Z

LibGit2Sharp.Tests/MetaFixture.cs

+
+            foreach (var method in mis.Distinct())
+            {
+                sb.AppendFormat("At least one overload of method '{0}' accepts an optional paramater.{1}",


typo: parameter

nulltoken · 2015-04-25T19:01:09Z

@shiftkey Any help is welcome killing those optional parameters! 🙏

shiftkey · 2015-04-25T19:08:08Z

@nulltoken I'll start from the bottom of the list then. Shall I PR into this PR, or just add commits directly?

nulltoken · 2015-04-25T19:39:26Z

@shiftkey Please push directly on top of this branch. ❤️ for the help

shiftkey · 2015-04-25T19:44:48Z

LibGit2Sharp.Tests/MetaFixture.cs

+                from t in Assembly.GetAssembly(typeof(IRepository))
+                    .GetExportedTypes()
+                from m in t.GetMethods()
+                where !m.IsObsolete()


There's a couple of methods which have optional parameters but are marked as obsolete. I figure we don't care about migrating those. Let me know if that's not the case.

Good call! They'll be dropped before stabilization (post v0.22)

shiftkey · 2015-04-25T22:12:53Z

LibGit2Sharp/Configuration.cs

        /// <param name="xdgConfigurationFileLocation">Path to a XDG configuration file. If null, the default path for a XDG configuration file will be probed.</param>
        /// <param name="systemConfigurationFileLocation">Path to a System configuration file. If null, the default path for a system configuration file will be probed.</param>
-        public Configuration(string globalConfigurationFileLocation = null, string xdgConfigurationFileLocation = null, string systemConfigurationFileLocation = null)
+        public Configuration(string globalConfigurationFileLocation, string xdgConfigurationFileLocation, string systemConfigurationFileLocation)


Not sure if we wanted to overload everything here - I just went with the overloads for global and global/xdg. Would love some guidance.

How you did it makes sense to me.

shiftkey · 2015-04-25T22:37:51Z

Pausing here because I'm concerned about wrestling with the mocks for Configuration and Doing It Wrong™ - the new tests however are passing, and I Have Questions™ about the ctor overloads for Configuration.

jamill · 2015-04-28T03:39:13Z

😍

shiftkey · 2015-04-30T22:36:33Z

@nulltoken thoughts on what I'm doing wrong with that leaked ConfigurationSafeHandle - I can't seem to trigger this when running the CanFakeEnumerationOfConfiguration test locally?

jamill · 2015-05-01T02:04:40Z

@shiftkey see this line - In order to trigger this, you need to have LEAKS_IDENTIFYING defined. uncomment that line, or add that symbol. Here is the trace with LEAKS_TRACKING specified:

at LibGit2Sharp.Core.NativeMethods.git_config_new(ConfigurationSafeHandle& cfg)
at LibGit2Sharp.Core.Proxy.git_config_new() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Core\Proxy.cs:line 482
at LibGit2Sharp.Configuration.Init() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 46
at LibGit2Sharp.Configuration..ctor(Repository repository, String globalConfigurationFileLocation, String xdgConfigurationFileLocation, String systemConfigurationFileLocation) in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 41
at LibGit2Sharp.Configuration..ctor(String globalConfigurationFileLocation, String xdgConfigurationFileLocation, String systemConfigurationFileLocation) in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp\Configuration.cs:line 100
at Castle.Proxies.ConfigurationProxy..ctor(IInterceptor[] , String , String , String )
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes, StackCrawlMark& stackMark)
at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
at System.Activator.CreateInstance(Type type, Object[] args)
at Castle.DynamicProxy.ProxyGenerator.CreateClassProxyInstance(Type proxyType, List`1 proxyArguments, Type classToProxy, Object[] constructorArguments)
at Castle.DynamicProxy.ProxyGenerator.CreateClassProxy(Type classToProxy, Type[] additionalInterfacesToProxy, ProxyGenerationOptions options, Object[] constructorArguments, IInterceptor[] interceptors)
at Moq.Proxy.CastleProxyFactory.CreateProxy(Type mockType, ICallInterceptor interceptor, Type[] interfaces, Object[] arguments)
at Moq.Mock`1.<InitializeInstance>b__2()
at Moq.PexProtector.Invoke(Action action)
at Moq.Mock`1.InitializeInstance()
at Moq.Mock`1.OnGetObject()
at Moq.Mock`1.get_Object()
at LibGit2Sharp.Tests.MockingFixture.CanFakeEnumerationOfConfiguration() in C:\Users\jamill\gitWorkspaces\libgit2sharp\LibGit2Sharp.Tests\MockingFixture.cs:line 97
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
at Xunit.Sdk.Reflector.ReflectionMethodInfo.Invoke(Object testClass, Object[] parameters)
at Xunit.Sdk.FactCommand.Execute(Object testClass)
at Xunit.Sdk.FixtureCommand.Execute(Object testClass)
at Xunit.Sdk.BeforeAfterCommand.Execute(Object testClass)
at Xunit.Sdk.LifetimeCommand.Execute(Object testClass)
at Xunit.Sdk.ExceptionAndOutputCaptureCommand.Execute(Object testClass)
at Xunit.Sdk.TimedCommand.Execute(Object testClass)
at Xunit.Sdk.TestClassCommandRunner.Execute(ITestClassCommand testClassCommand, List`1 methods, Predicate`1 startCallback, Predicate`1 resultCallback)
at Xunit.Sdk.Executor.RunTests.<>c__DisplayClass12.<.ctor>b__f()
at Xunit.Sdk.Executor.ThreadRunner(Object threadStart)
at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.ThreadHelper.ThreadStart(Object obj)

jamill · 2015-05-01T02:41:18Z

@shiftkey - I am not completely familiar with this yet, but it looks like the Mock<Config> object is calling the constructor for the class, which in turn allocates a new ConfigurationSafeHandle. My (possibly wrong) expectation is that we do not want the mock to actually initialize itself to this extent? Having the mock call into libgit2 and set up some native structures seems to break the encapsulation of the mock object - maybe we want an IConfig interface?

shiftkey · 2015-05-07T08:46:32Z

Okay, that should be the fix. I'll try and give it thorough review to ensure the optional combinations are maintained wherever appropriate, but any extra help would be 💖

shiftkey · 2015-05-07T08:52:44Z

nulltoken · 2015-05-09T08:15:19Z

LibGit2Sharp.sln.DotSettings

@@ -10,6 +10,7 @@
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_AFTER_TYPECAST_PARENTHESES/@EntryValue">False</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_WITHING_EMPTY_BRACES/@EntryValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_WITHIN_SINGLE_LINE_ARRAY_INITIALIZER_BRACES/@EntryValue">True</s:Boolean>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateInstanceFields/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;</s:String>


What does this change do? (asking for a friend)

I think I found myself dropping the default _ when R# was bugging me during the refactoring. I'll check.

nulltoken · 2015-05-14T16:56:32Z

Pushed some fixups

nulltoken · 2015-05-15T08:03:35Z

Anyone opposing to see this pulled in?

Therzok · 2015-05-15T11:14:26Z

Ensure lack of optional parameters

nulltoken · 2015-05-15T18:03:55Z

@shiftkey AWE. SOME! 💯 🎱

nulltoken added the Stabilization label Apr 25, 2015

shiftkey reviewed Apr 25, 2015
View reviewed changes

shiftkey force-pushed the ntk/optionals branch from abff0ec to 069fc5e Compare April 25, 2015 19:49

shiftkey reviewed Apr 25, 2015
View reviewed changes

shiftkey force-pushed the ntk/optionals branch from 0fb52f9 to 913542a Compare April 30, 2015 22:03

shiftkey force-pushed the ntk/optionals branch from 06c9e4b to 9620815 Compare May 7, 2015 08:45

nulltoken reviewed May 9, 2015
View reviewed changes

nulltoken force-pushed the ntk/optionals branch from 3662430 to f5c6a9c Compare May 10, 2015 11:10

shiftkey added 20 commits May 15, 2015 19:31

Drop optional parameters in TagCollection

494de48

Drop optional parameters in RepositoryExtensions

14b3c1a

Drop optional parameters in Repository.cs

a897011

Drop optional parameters in RemoteCollection.cs

cb9ebe5

Drop optional parameters in ReferenceCollectionExtensions.cs

0a030f8

Drop optional parameters in ObjectDatabase.cs

1bb9d42

Drop optional parameters in TagCollectionExtensions.cs

d16fbec

Drop optional parameters in ReferenceCollectionExtensions.cs

bce3f4c

Drop optional parameters in NetworkExtensions.cs

631b36c

Drop optional parameters in ConfigurationExtensions.cs

4277559

Drop optional parameters in Configuration.cs

9b26f36

Drop optional parameters in StashCollection.cs

1f9d9dd

Drop optional parameters in Network.cs

0cfa923

Drop optional parameters in CommitRewriteInfo.cs

d7e8a2e

Drop optional parameters in BranchCollectionExtensions.cs

b3386c9

Drop optional parameters in BranchCollection.cs

40bbf1e

Drop optional parameters in BlobExtensions.cs

32da01c

Drop optional parameters in Diff.cs

44aa149

Ensure lack of optional parameters

5109f59

Fix Resharper private member prefix setting

62cc73e

nulltoken force-pushed the ntk/optionals branch from 0fbacd3 to 62cc73e Compare May 15, 2015 17:33

nulltoken added a commit that referenced this pull request May 15, 2015

Merge pull request #1031 from libgit2/ntk/optionals

98e6e34

Ensure lack of optional parameters

nulltoken merged commit 98e6e34 into vNext May 15, 2015

nulltoken deleted the ntk/optionals branch May 15, 2015 18:02

nulltoken added this to the v0.22 milestone May 15, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure lack of optional parameters #1031

Ensure lack of optional parameters #1031

nulltoken commented Apr 25, 2015

shiftkey commented Apr 25, 2015

shiftkey Apr 25, 2015

nulltoken commented Apr 25, 2015

shiftkey commented Apr 25, 2015

nulltoken commented Apr 25, 2015

shiftkey Apr 25, 2015

nulltoken Apr 25, 2015

shiftkey Apr 25, 2015

jamill Apr 28, 2015

shiftkey May 1, 2015

shiftkey commented Apr 25, 2015

jamill commented Apr 28, 2015

shiftkey commented Apr 30, 2015

jamill commented May 1, 2015

jamill commented May 1, 2015

shiftkey commented May 7, 2015

shiftkey commented May 7, 2015

nulltoken May 9, 2015

shiftkey May 9, 2015

nulltoken commented May 14, 2015

nulltoken commented May 15, 2015

Therzok commented May 15, 2015

nulltoken commented May 15, 2015

Ensure lack of optional parameters #1031

Ensure lack of optional parameters #1031

Conversation

nulltoken commented Apr 25, 2015

shiftkey commented Apr 25, 2015

Choose a reason for hiding this comment

nulltoken commented Apr 25, 2015

shiftkey commented Apr 25, 2015

nulltoken commented Apr 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Apr 25, 2015

jamill commented Apr 28, 2015

shiftkey commented Apr 30, 2015

jamill commented May 1, 2015

jamill commented May 1, 2015

shiftkey commented May 7, 2015

shiftkey commented May 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented May 14, 2015

nulltoken commented May 15, 2015

Therzok commented May 15, 2015

nulltoken commented May 15, 2015