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

Split namespaces into Different Libraries. #3136

Merged
merged 13 commits into from
Feb 15, 2024

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Feb 12, 2024

Description

This is a small change for now. A lot more changes to come in future PRs. But this will force everyone to organize their code to the appropriate libraries. All namespace, class, methods and etc are all the same. The only difference is they may have moved to a different library. In addition, local or global variable names may have changed; these changes do not affect the functionality in any way; just following our coding practices.

Please Note: This will break neo-modules. We will need to update after this PR is merged. Nevermind this won't break them unless you don't import from neo.dll

Libraries

  • Neo.IO
    • All things IO
  • Neo.Extensions
    • All things that extend functionality of neo. Including but not limited to other libraries. No test functionality allowed. Please Note: you can include this library in tests; just don't add code that extends tests.

Type of change

  • 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)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cschuchardt88 cschuchardt88 marked this pull request as ready for review February 12, 2024 20:06
@cschuchardt88
Copy link
Member Author

@shargon @Jim8y

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Thanks for make this in small pr

src/Neo.IO/Neo.IO.csproj Show resolved Hide resolved
src/Neo.IO/ByteArrayComparer.cs Outdated Show resolved Hide resolved
src/Neo.Extensions/Neo.Extensions.csproj Show resolved Hide resolved
: -CompareInternal(x, y);
if (ReferenceEquals(x, y)) return 0;
if (x is null && y is not null) return -y.Length;
if (y is null && x is not null) return x.Length;
Copy link
Member

Choose a reason for hiding this comment

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

If x is empty, it will return 0 that means is the same, I think that it's better to return -1 and +1 only when null

Copy link
Member Author

@cschuchardt88 cschuchardt88 Feb 13, 2024

Choose a reason for hiding this comment

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

Now tell if i'm wrong; i sometimes have trouble with stuff like this.

Currently flow goes

  1. if x is same instance as y return 0
  2. if x is null and y is not null then return x is less than y length.
  3. if y is null and x is not null then return x is greater than y length
  4. if x and y is not null then return the comparison
  5. the only thing left is: if x and y is null then return 0 aka the same.

if (ReferenceEquals(x, y)) return 0;
if (x is null && y is not null) return -y.Length;
if (y is null && x is not null) return x.Length;
if (x is not null && y is not null)
Copy link
Member

Choose a reason for hiding this comment

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

Is imposible to be null x and y here, we can remove this if

Copy link
Member Author

@cschuchardt88 cschuchardt88 Feb 13, 2024

Choose a reason for hiding this comment

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

if they both null it return 0 its the last line in the method.

Copy link
Member

Choose a reason for hiding this comment

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

But we check if both are null twice

        public int Compare(byte[]? x, byte[]? y)
        {
            if (ReferenceEquals(x, y)) return 0; // x == y || x == null && y == null
            if (x is null && y is not null)
                return _direction > 0 ? -y.Length : y.Length;
            if (y is null && x is not null)
                return _direction > 0 ? x.Length : -x.Length;

#pragma warning disable CS8604 // Possible null reference argument.
            return _direction > 0 ?
                CompareInternal(x, y) :
                -CompareInternal(x, y);
#pragma warning restore CS8604 // Possible null reference argument.
        }

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 13, 2024

@shargon All test pass. Updated unit tests for ByteArrayComparer to test changes made.

image

@@ -31,7 +31,7 @@ public Logger()
}
}

public static event LogEventHandler Logging;
Copy link
Member

@shargon shargon Feb 13, 2024

Choose a reason for hiding this comment

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

Use Neo.Extensions in Neo.Vm to avoid duplicate Neo.VM.Utility.StrictUTF8? This can be done later.

@@ -12,6 +12,7 @@
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Neo.IO;
using System;

namespace Neo.UnitTests.IO
Copy link
Member

@shargon shargon Feb 13, 2024

Choose a reason for hiding this comment

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

We should create a test project to Neo.IO and move these test to that project.

Copy link
Member Author

@cschuchardt88 cschuchardt88 Feb 13, 2024

Choose a reason for hiding this comment

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

I planning on it.

Copy link
Member

Choose a reason for hiding this comment

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

Why not in the same PR?

Copy link
Member Author

@cschuchardt88 cschuchardt88 Feb 15, 2024

Choose a reason for hiding this comment

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

This PR moves existing code to new library. There is or should be tests already created. This PR also doesn't change anything for application besides, now having an extra dll. In the next few PRs will be moving and checking for tests, if none than iIl create them. Just want to get the library projects started and not be to big.

@shargon
I can create one. If you would like.

if (ReferenceEquals(x, y)) return 0;
if (x is null && y is not null) return -y.Length;
if (y is null && x is not null) return x.Length;
if (x is not null && y is not null)
Copy link
Member

Choose a reason for hiding this comment

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

But we check if both are null twice

        public int Compare(byte[]? x, byte[]? y)
        {
            if (ReferenceEquals(x, y)) return 0; // x == y || x == null && y == null
            if (x is null && y is not null)
                return _direction > 0 ? -y.Length : y.Length;
            if (y is null && x is not null)
                return _direction > 0 ? x.Length : -x.Length;

#pragma warning disable CS8604 // Possible null reference argument.
            return _direction > 0 ?
                CompareInternal(x, y) :
                -CompareInternal(x, y);
#pragma warning restore CS8604 // Possible null reference argument.
        }

Copy link
Member Author

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

@shargon thanks for fixing that up. if statements lose me sometimes.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Feb 14, 2024

@Jim8y @shargon Can you review. This doesn't break anything; neo.dll imports these new libraries.

Also i need this to continue #3103 along with some other PRs i need to create after this. If it needs more testing, than lets merge to core-refactor branch instead.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 14, 2024

working on it.

@shargon
Copy link
Member

shargon commented Feb 14, 2024

@Jim8y @shargon Can you review. This doesn't break anything; neo.dll imports these new libraries.

Also i need this to continue #3103 along with some other PRs i need to create after this. If it needs more testing, than lets merge to core-refactor branch instead.

I was waiting this response #3136 (comment)

@shargon shargon merged commit 6746665 into neo-project:master Feb 15, 2024
2 checks passed
@shargon shargon deleted the refactor branch February 15, 2024 07:48
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants