Refactor System.IO.Path

[CoryCharlton]

Description

• Bring code in line with .NET full implementation
• Modernize code structure
• Add unit test coverage
• Optimize performance

Motivation and Context

The System.IO.FileSystem library hasn't got love in a while so this was another opportunity to clean up the code and confirm we are following the implementations in .NET full

How Has This Been Tested?

• Unit tests
• On device

Screenshots

Types of changes

• Improvement (non-breaking change that improves a feature, code or algorithm)
• Bug fix (non-breaking change which fixes an issue with code or algorithm)
• New feature (non-breaking change which adds functionality to code)
• Breaking change (fix or feature that would cause existing functionality to change)
• Config and build (change in the configuration and build system, has no impact on code or features)
• Dependencies (update dependencies and changes associated, has no impact on code or features)
• Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
• Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

• My code follows the code style of this project (only if there are changes in source code).
• My changes require an update to the documentation (there are changes that require the docs website to be updated).
• I have updated the documentation accordingly (the changes require an update on the docs in this repo).
• I have read the CONTRIBUTING document.
• I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
• I have added new tests to cover my changes.

[josesimoes]

Wow! That's a lot of improvements.
Wondering if we really need support for UNC paths... 🤔
I can't think of a use case in nano that can use it...

In order to keep libraries and firmware as small as possible, we heigth the cost/benefit/usage before adding new stuff. Just because we can, we don't rush into adding it.

Please don't get me wrong, I'm not picking on your work here.
That's why we hav on the contribution guideline the suggestion to first discuss changes and new code before actually start active work on it. In order to save time (and frustration) 😉

Open for discussion.

[CoryCharlton]

Wow! That's a lot of improvements. Wondering if we really need support for UNC paths... 🤔 I can't think of a use case in nano that can use it...

In order to keep libraries and firmware as small as possible, we heigth the cost/benefit/usage before adding new stuff. Just because we can, we don't rush into adding it.

Please don't get me wrong, I'm not picking on your work here. That's why we hav on the contribution guideline the suggestion to first discuss changes and new code before actually start active work on it. In order to save time (and frustration) 😉

Open for discussion.

Makes perfect sense and I actually questioned the UNC support while I was copying it over.

Speaking of copying it over the majority of this work came directly from the .NET Core implementation and so from that aspect it was easier for me to copy as is and trim stuff out from there. This allowed me to implement the unit tests in a .NET Core project, from a known set of behaviors, and confirm they ran the same in the nanoFramework implementation.

My motivation for starting down this Path (😉) was that the old implementation did not support the alternative directory separator ('/') so combining relative URLs with filesystem paths did not work like I expected. Then when I started digging into the code I saw room to remove unnecessary overhead (ex: checking for invalid filesystem characters which shouldn't be a concern of this class).

Happy to trim out things we don't need and will start with the UNC support.

[CoryCharlton]

I started looking at the UNC handling and it looks like the old code had, at least partial, support:

System.IO.FileSystem/System.IO.FileSystem/Path.cs

Line 170 in 51b40b7

// Handles UNC names and directories off current drive's root.

System.IO.FileSystem/System.IO.FileSystem/Path.cs

Line 481 in 51b40b7

bool rootedPath = false;

Additionally at first glance even though the new code is ~50 lines more (a poor metric I know) it appears to be much smaller (top is the latest NuGet and bottom is this branch build in release mode):

I'll investigate this a little further but it appears this gets things in line with the .NET Full behavior and doesn't add anything additional space wise.

[CoryCharlton]

Character wise (I know, not a real metric) if I remove comments the old implementation is 14,263 characters and the new one is 14,154 (manual removal so some margin of error ;)). Long story short the only thing added was correct handling of all expected cases and I think we're good to go unless I missed something.

PS: The SonarCloud bug is not actually a bug since our string doesn't implement GetEnumerator() I had to use ToCharArray(). I could switch to a for ... i I suppose.

[Ellerbach]

PS: The SonarCloud bug is not actually a bug since our string doesn't implement GetEnumerator() I had to use ToCharArray(). I could switch to a for ... i I suppose

I marked it in Sonar, should be good for the next time you'll push anything

[Ellerbach]

couple of comments and bonus point for the tests!

[josesimoes]

@CoryCharlton not arguing that it wasn't there before. 😄 Rather if we need it.
If there is code, there is flash/RAM space taken. No matter if it's C# only, has native code backing it or not. 😉

[josesimoes]

For the record: internal discussion started to gather thoughts on this UNC matter).

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

[josesimoes]

LGTM! Thanks 👍🏻