Simplify data structure #266

Arkatufus · 2020-03-06T15:45:47Z

Remove implicit casting

# Conflicts: # src/Hocon.API.Tests/HoconAPISpec.ApproveConfiguration.approved.txt # src/Hocon.Configuration/Config.cs # src/Hocon.Configuration/HoconConfigurationFactory.cs

…ows SDK version.

…n irrepairable critical error.

# Conflicts: # Hocon.sln

…n in Akka

…m running.

…eflects the memory cost of the lookup spec

…me refactor

# Conflicts: # src/Hocon.API.Tests/HoconAPISpec.ApproveCore.approved.txt # src/Hocon/HoconObject.cs

…into Simplify_Data_Structure

…_Data_Structure_Fast_Forwarded

…ure_Fast_Forwarded # Conflicts: # src/Hocon.Configuration/HoconConfigurationFactory.cs

Aaronontheweb

Some changes, some questions

Aaronontheweb · 2020-03-06T17:44:49Z

src/Hocon.API.Tests/Hocon.API.Tests.csproj

@@ -45,7 +45,6 @@
  <ItemGroup>
    <ProjectReference Include="..\Hocon.Configuration\Hocon.Configuration.csproj" />
    <ProjectReference Include="..\Hocon.Extensions.Configuration\Hocon.Extensions.Configuration.csproj" />
-    <ProjectReference Include="..\Hocon.Immutable\Hocon.Immutable.csproj" />


Yep, good to see Hocon.Immutable getting disposed

Aaronontheweb · 2020-03-06T17:45:09Z

src/Hocon.API.Tests/HoconAPISpec.ApproveConfiguration.approved.txt

        public static Hocon.Config Empty { get; }
-        public virtual System.Collections.Generic.IReadOnlyList<Hocon.HoconValue> Fallbacks { get; }
+        public virtual System.Collections.Generic.IReadOnlyList<Hocon.HoconObject> Fallbacks { get; }


What's the significance of this change?

There's no such thing as HoconValue anymore, the structure is simplified.

Aaronontheweb · 2020-03-06T17:45:29Z

src/Hocon.API.Tests/HoconAPISpec.ApproveConfiguration.approved.txt

        public string ToString(bool useFallbackValues) { }
-        protected override bool TryGetNode(Hocon.HoconPath path, out Hocon.HoconValue result) { }
+        public override bool TryGetValue(Hocon.HoconPath path, out Hocon.HoconElement result) { }


Same as above - what's the significance of this change too?

Same as the above, there is no HoconValue class.

Aaronontheweb · 2020-03-06T17:48:01Z

src/Hocon.API.Tests/HoconAPISpec.ApproveCore.approved.txt

    }
-    public sealed class HoconBool : Hocon.HoconLiteral
+    public sealed class HoconArrayBuilder : System.Collections.Generic.List<Hocon.HoconElement>


Like the idea of using the builder pattern to build up the HOCON, but we need to avoid inheriting directly from mutable base classes

There is no danger, the builder pattern is only used for building the hocon classes, all the returned built classes are immutable.

Aaronontheweb · 2020-03-06T17:48:54Z

src/Hocon.Configuration.Test/ConfigurationSpec.cs

@@ -31,19 +31,6 @@ public class MyObjectConfig
            public int[] IntergerArray { get; set; }
        }

-        [Fact]
-        public void Config_should_be_serializable()


Fine deferring this for now

This is moved to SerializationSpec. Config does not support direct JSON serialization anymore, because of immutability.

Aaronontheweb · 2020-03-06T17:54:05Z

src/Hocon/Builder/HoconObjectBuilder.cs

+
+namespace Hocon
+{
+    public sealed class HoconObjectBuilder : SortedDictionary<string, HoconElement>


Same as above - might want to make internal

Aaronontheweb · 2020-03-06T17:55:22Z

src/Hocon/HoconObject.cs

+
+        public new HoconElement this[string path] {
+            get => GetValue(path);
+            set => throw new InvalidOperationException("HoconObject is a read only Dictionary.");


Can we avoid this by just not inheriting from IDictionary - leave it as IReadOnlyDictionary only? Or did you need this for testing?

IDictionary interface is needed for backward compatibility, I've blocked all of the write behaviour

Aaronontheweb · 2020-03-06T17:58:33Z

src/Hocon.Configuration/Config.cs

+            if(root is Config cfg)
+            {
+                Root = cfg.Root;
+                _fallbacks = cfg._fallbacks.ToList();


Why is the ToList() call necessary here? Can the cfg._fallbacks property be modified?

Yes, each Config instance need to have their own separate instance of fallbacks, else changing (appending a new fallback value in WithFallback) will affect ALL Config objects before it.

Aaronontheweb · 2020-03-06T17:59:10Z

src/Hocon.Configuration/Config.cs

-        protected List<HoconValue> _fallbacks { get; } = new List<HoconValue>();
-        public virtual IReadOnlyList<HoconValue> Fallbacks => _fallbacks.ToList().AsReadOnly();
+        protected List<HoconObject> _fallbacks { get; } = new List<HoconObject>();
+        public virtual IReadOnlyList<HoconObject> Fallbacks => _fallbacks.ToList().AsReadOnly();


No need to call ToList().AsReadOnly() - List<T> is castable to IReadOnlyList<T>

Defensive coding, I want to be 100% sure that the user could not change the internal representation of the fallback list.

Aaronontheweb · 2020-03-06T17:59:46Z

src/Hocon.Configuration/Config.cs


            if (fallback.IsNullOrEmpty())
                return this; // no-op

            if (IsEmpty)
                return fallback;

-            return new Config(this, fallback);
+            var result = new Config(this);
+            result.MergeConfig(fallback);


So this just appends the latest fallback to what position on the linked list?

fallbacks are always added to the back of the chain

Aaronontheweb · 2020-03-06T18:02:20Z

Going to do a perf comparison in a second - stay tuned

Aaronontheweb · 2020-03-06T18:17:49Z

Sigh, all of my performance comparison posts got added to here by accident: akkadotnet/akka.net#4301 (comment)

Performance in this PR is looking good.

Arkatufus added 30 commits February 20, 2020 20:56

Move Hocon.Immutable into Hocon namespace

84a3c5a

Mark old classes as internal classes

2eedda3

Convert old data type to new ones

280a826

Convert old data type to new data type

2e09c9d

Improve backward compatibility

84dd12c

Convert old Config to use the new data structure.

0b7beb2

Add caching to Config

2d5e60c

Update ApiApprover text file to reflect the new data structure.

10d2659

Merge branch 'dev' into Simplify_Data_Structure

2ea0ce5

# Conflicts: # src/Hocon.API.Tests/HoconAPISpec.ApproveConfiguration.approved.txt # src/Hocon.Configuration/Config.cs # src/Hocon.Configuration/HoconConfigurationFactory.cs

Bump Linux .NET SDK version from 2.1.500 to 2.1.504 to match the wind…

b5a687e

…ows SDK version.

force csproj LangVersion tag to 7.2

8c4a583

Remove throw statement, throw does not make sense since this is not a…

1c095b6

…n irrepairable critical error.

Add missing getters needed for Akka compatibility

debc2e6

Merge branch 'dev' into Simplify_Data_Structure

3ad3d69

# Conflicts: # Hocon.sln

Add serialization hacks (surrogate) from Akka.NET to fix serializatio…

279978f

…n in Akka

Merge branch 'dev' into Simplify_Data_Structure

8fa5415

Remove Newtonsoft.Json nuget dependency

cdc4f1c

Defensive coding

d91901c

Adjust surface API

1deb2a7

Clean ghost Immutable projects from .sln file. Prevented CI build fro…

ac1e4ad

…m running.

Prepend Hocon to classes names that are too generic

f9b4b75

Remove caching from Config

f109c44

Force garbage collection so that memory allocation stat result only r…

63305ed

…eflects the memory cost of the lookup spec

Update the approved API list to reflect the static extension class na…

9afc666

…me refactor

Add support for Hyperion deserialization

602409c

Update ApiApprover API list, need to add an [Obsolete] attribute

e62ac95

Merge branch 'dev' into Simplify_Data_Structure

49a1eaa

Merge branch 'dev' into Simplify_Data_Structure

4379001

# Conflicts: # src/Hocon.API.Tests/HoconAPISpec.ApproveCore.approved.txt # src/Hocon/HoconObject.cs

Merge branch 'Simplify_Data_Structure' of github.com:Arkatufus/HOCON …

6badd7d

…into Simplify_Data_Structure

Add local nuget repo publishing option to build script

298e57e

Arkatufus added 11 commits February 29, 2020 17:50

Merge commit '94981ce4be01537b1d45da91b3043944c810e151' into Simplify…

c4a0487

…_Data_Structure_Fast_Forwarded

Merge commit 'b4ad447b033a87271ee40fbadb575dfa6a519396' into Simplify…

f93814c

…_Data_Structure_Fast_Forwarded

Merge commit 'c88a902b11f3c9b097bcf90237da164d928ea458' into Simplify…

daf05f5

…_Data_Structure_Fast_Forwarded

Merge commit '298e57e97bbeb2b9b5fc8d99f02aa51af500837c' into Simplify…

7fdfbac

…_Data_Structure_Fast_Forwarded

Consistent equality between Config and HoconObject

3243d7a

Merge branch 'dev' into Simplify_Data_Structure_Fast_Forwarded

7eeba85

Bump version number to 3.0.0

d9f765e

Update RELEASE_NOTES.md

8755457

Remove implicit casting

e4d052d

Merge remote-tracking branch 'upstream/dev' into Simplify_Data_Struct…

f3ccb8a

…ure_Fast_Forwarded # Conflicts: # src/Hocon.Configuration/HoconConfigurationFactory.cs

Update ApiApprover API list

c815dde

Aaronontheweb requested changes Mar 6, 2020

View reviewed changes

Simplify data structure #266

Are you sure you want to change the base?

Simplify data structure #266

Uh oh!

Conversation

Arkatufus commented Mar 6, 2020

Uh oh!

Aaronontheweb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aaronontheweb commented Mar 6, 2020

Uh oh!

Aaronontheweb commented Mar 6, 2020

Uh oh!

Uh oh!