You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/07/16 21:25:03 UTC
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #823: GEODE-9360: Initial checkin of .net core support
pivotal-jbarrett commented on a change in pull request #823:
URL: https://github.com/apache/geode-native/pull/823#discussion_r671527167
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+using Apache.Geode.Client;
+using Xunit;
+
+namespace GeodeDotNetTest {
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests {
Review comment:
The naming convention used by the Java and C++ sources is `ClassTest(s)` where `Class` is the class under testing and `Test` or `Tests` (not consistent) is used to denote unit tests. Unit tests are also isolated away from other forms of longer feedback tests, like integration, also suffixing them `UnitTest` was deemed redundant.
##########
File path: .gitignore
##########
@@ -18,3 +18,7 @@
/tools/gnmsg/.idea/
/tools/gnmsg/__pycache__/
+
+/netcore/*/bin/
Review comment:
Is there any way to get .NET build tools to not build in the source tree?
##########
File path: netcore/.clang-format
##########
@@ -0,0 +1,3 @@
+---
+Language: CSharp
+BasedOnStyle: Google
Review comment:
YES!
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+using Apache.Geode.Client;
+using Xunit;
+
+namespace GeodeDotNetTest {
+ [Collection("Geode .net Core Collection")]
+ public class CacheFactoryUnitTests {
+ [Fact]
+ public void TestCreateFactory() {
Review comment:
Since all these methods are tests, does it make sense to redundantly prefix them all with `Test`? If read in plain English you have "Fact, test create factory." This is is not a complete fact. More appropriately it should be called `CacheFactoryCreateIsNotNull` read as "Fact, CacheFactory.Create is not null", which is an asserted fact of this test.
##########
File path: netcore/NetCore/IRegionService.cs
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+
+namespace Apache {
+ namespace Geode {
+ namespace Client {
+ public interface IRegionService {
+ // IPdxInstanceFactory CreatePdxInstanceFactory(String className);
Review comment:
Delete commented out code.
##########
File path: netcore/scripts/run-clang-format.cmd
##########
@@ -0,0 +1,16 @@
+rem Licensed to the Apache Software Foundation (ASF) under one or more
+rem contributor license agreements. See the NOTICE file distributed with
+rem this work for additional information regarding copyright ownership.
+rem The ASF licenses this file to You under the Apache License, Version 2.0
+rem (the "License"); you may not use this file except in compliance with
+rem the License. You may obtain a copy of the License at
+rem
+rem http://www.apache.org/licenses/LICENSE-2.0
+rem
+rem Unless required by applicable law or agreed to in writing, software
+rem distributed under the License is distributed on an "AS IS" BASIS,
+rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+rem See the License for the specific language governing permissions and
+rem limitations under the License.
+clang-format -i --style=file NetCore/*.cs
Review comment:
Why not a build step in CMake?
##########
File path: .gitignore
##########
@@ -18,3 +18,7 @@
/tools/gnmsg/.idea/
/tools/gnmsg/__pycache__/
+
+/netcore/*/bin/
Review comment:
Also, can you put these in a `.gitignore` in the `netcore` directory.
##########
File path: netcore/NetCore/Cache.cs
##########
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+namespace Apache {
Review comment:
Can we use `namespace Apahce.Geode.Client.Cache` convention that is more popular in C#. This also conserves a lot of whitespace.
##########
File path: netcore/NetCore.Test/CacheFactoryUnitTest.cs
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+using Apache.Geode.Client;
+using Xunit;
+
+namespace GeodeDotNetTest {
Review comment:
Unit tests should really be in the same namespace as the thing they are testing.
##########
File path: netcore/NetCore.Test/xunit.runner.json
##########
@@ -0,0 +1,5 @@
+{
+ "methodDisplay": "classAndMethod",
+ "parallelizeAssembly": true,
+ "parallelizeTestCollections": true
+}
Review comment:
Missing terminating new lines in multiple files.
##########
File path: netcore/geode-dotnet-core.sln
##########
@@ -0,0 +1,31 @@
+
+Microsoft Visual Studio Solution File, Format Version 12.00
+# Visual Studio Version 16
+VisualStudioVersion = 16.0.30001.183
+MinimumVisualStudioVersion = 10.0.40219.1
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NetCore", "NetCore\NetCore.csproj", "{09ABBCE7-B217-43F1-A51B-CC5BDCD8EE98}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NetCore.Test", "NetCore.Test\NetCore.Test.csproj", "{501DEA7E-8985-42A8-8BC9-C073E1B6DFE0}"
+EndProject
+Global
+ GlobalSection(SolutionConfigurationPlatforms) = preSolution
+ Debug|x64 = Debug|x64
+ Release|x64 = Release|x64
+ EndGlobalSection
+ GlobalSection(ProjectConfigurationPlatforms) = postSolution
+ {09ABBCE7-B217-43F1-A51B-CC5BDCD8EE98}.Debug|x64.ActiveCfg = Debug|x64
+ {09ABBCE7-B217-43F1-A51B-CC5BDCD8EE98}.Debug|x64.Build.0 = Debug|x64
+ {09ABBCE7-B217-43F1-A51B-CC5BDCD8EE98}.Release|x64.ActiveCfg = Release|x64
+ {09ABBCE7-B217-43F1-A51B-CC5BDCD8EE98}.Release|x64.Build.0 = Release|x64
+ {501DEA7E-8985-42A8-8BC9-C073E1B6DFE0}.Debug|x64.ActiveCfg = Debug|x64
+ {501DEA7E-8985-42A8-8BC9-C073E1B6DFE0}.Debug|x64.Build.0 = Debug|x64
+ {501DEA7E-8985-42A8-8BC9-C073E1B6DFE0}.Release|x64.ActiveCfg = Release|x64
Review comment:
In C++ and .NET Framework we are building RelWithDebInfo to both optimize for release and produce debug information. Does the C# Release mode conserve debug info?
##########
File path: netcore/NetCore.Test/CacheUnitTest.cs
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+using System;
+using System.Net.Cache;
+using Apache.Geode.Client;
+using Xunit;
+
+namespace GeodeDotNetTest {
+ [Collection("Geode .net Core Collection")]
+ public class CacheUnitTests {
+ [Fact]
+ public void TestClientCacheGetPdxReadSerialized() {
+ using var cacheFactory =
+ CacheFactory.Create()
Review comment:
Why is `new CacheFactory` not sufficient that we need a `CacheFactory.Create()` method, which is confusing with the instance method, `CreateCache`. In the normal builder pattern like this you would have `new CacheFactory().somthing().somthingElse().create()` to create the thing the factory was building.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org