RevEng: Canonicalizing output paths

[lajones]

Fix for #3830. For Scaffold-DbContext we send back a list of the paths to the generated files to powershell so that it can update the VS project to include them in the project. When the output directory looked like ".\Models" we were sending back filenames like "X:\Project\Path.\Models\MyEntity.cs". The extra "." was being interpreted by the VS AddFromFile() API as a real part of the name of the file. This fixes that by canonicalizing the name of the path before it get sent back so that the above now becomes "X:\Project\Path\Models\MyEntity.cs".

[divega]

Why repeat the words of the name of the class in each property?

[lajones]

Will change them.

[divega]

I might be wrong, but a quick look at the change gives me the impression that something simpler could work and be more maintainable. I would the usual reviewers for thus area to chip in though. Cc @natemcmaster, @bricelam

[divega]

@lajones note that the Travis build for this PR is failing. @natemcmaster noticed last night that it seems to be a legitimate issue with the PR. Apparently either something in the new path handling logic or on one of the new tests doesn't work well in non-Windows platforms.

[lajones]

@divega - feel free to suggest. The only thing I've found that canonicalizes is Path.GetFullPath(). And this algorithm was already in use to produce the namespace - I've just adapted it so we can use it both for the output paths and the namespace. I would have preferred out params and skipping the CanonicalizedOutputPaths class altogether, but that's against the current coding practice. Have added new commit 7e7a205 which combines ConstructOutputPaths() and ConstructNamespace() to make it simpler.

[natemcmaster]

The failing test on Linux is:

ReverseEngineeringGeneratorTests.Constructs_correct_canonical_paths

rootNamespace: "Root.Namespace", 
projectPath: "/project/Path", 
outputPath: "", 
expectedNamepace: "Root.Namespace", 
canonicalizedFullOutputPath: "project/Path", 
canonicalizedRelativeOutputPath: ""

      Assert.Equal() Failure
                ↓ (pos 0)
      Expected: project/Path
      Actual:   /project/Path
                ↑ (pos 0)

[natemcmaster]

Was this check intentionally removed?

[lajones]

Nope - well spotted. Will put back in.

[lajones]

@natemcmaster re Linux test: typo on my end. Should be fixed with commit c698808. Let me know if not.

[natemcmaster]

Not completely sure if this is the same, but this looks almost the same as Uri.MakeRelativeUri

[lajones]

I tried Uri stuff a while back. Don't remember exactly what the problem was now but would rather keep full control over what we are doing.

[natemcmaster]

Could we use Uri.Segments to make this more x-plat safe?

[lajones]

Hmmm. It seems to be passing on both Linux and Windows platforms already? I'd rather not complicate it unless I have to.

[natemcmaster]

Per @divega about simplifying, here are a few suggestions:

• ConstructNamespaceAndCanonicalizedPaths can be split into separate functions. These are two separate concerns.
• The class NamespaceAndOutputPaths could be a tuple. This is never used by other components.
• Reuse functionality in System.Uri (where possible.)

[lajones]

@natemcmaster @divega actually ConstructNamespaceAndCanonicalizedPaths was 2 separate functions in the 1st commit - I put them together to simplify. And they are not unrelated - you need the canonicalized relative (both the canonicalized and the relative are important) path to construct the namespace. If you put them as 2 separate functions then either you have to pass the canonicalized relative path into the ConstructNamespace function - which makes it odd as a public standalone method - how would you check that what is passed in is canonicalized, what would you do if it wasn't relative? Or you can pass ConstructNamespace the same args as ConstructOutputPaths (plus rootNamespace) and call ConstructOutputPaths from ConstructNamespace to make sure you get the canonicalized relative path but then you would just be calling the same function twice on the same data? It seemed more efficient to just do it all at once.

I agree it could return a Tuple - to me that's actually less clear than what I've done here but I will do it if it makes it clearer to others.

Re Uri - I just did a little testing and, without using Path.GetFullPath(), Uris start throwing UriFormatException if they come across the "." or ".." syntax. So would much rather use Path.

Re overall simplicity: ConstructNamespaceAndCanonicalizedPaths seems to me to be a pretty simple little helper function very similar to what was in ConstructNamespace before. Agree it is very specific to what it does and is not used elsewhere - I would have liked to make it private but I kept it public so we could test it. The rest is just updates to the code to use ConstructNamespaceAndCanonicalizedPaths plus updates to tests plus removing some whitespace which I came across while debugging this.

[natemcmaster]

I agree it could return a Tuple - to me that's actually less clear than what I've done here but I will do it if it makes it clearer to others.

Agreed. Anonymous types or private type instead? A public type adds API surface for something that isn't an API. NamespaceAndOutputPaths is really just a named data structure for marshaling data from one function to another.

ConstructNamespaceAndCanonicalizedPaths was 2 separate functions in the 1st commit - I put them together to simplify

I still suggest splitting. From a design perspective, constructing a namespace and canonicalizing paths are separate responsibilities. Avoiding a precondition check is not reason enough to push them together.

I would have liked to make it (ConstructNamespaceAndCanonicalizedPaths) private but I kept it public so we could test it.

Make it internal so it can still be tested but is not part of our public API. We should do this regardless of how the "split or don't" decision is solve above.

[lajones]

@natemcmaster Happy to do internal. And will see what I can do to make NamespaceAndOutputPaths not public but still testable. Still disagree on splitting the methods. To construct the namespace you need the canonicalized relative path. The only "precondition check" I could imagine putting in would be to also pass it the projectPath and outputPath and going through the same functionality as before to check that what has been passed in is the correct canonicalized relative path. At that point I might as well not bother passing the canonicalized relative path at all but just call a new ConstructCanonicalizedPaths internally from within ConstructNamespace and we'll be repeating the same code calculations. Not sure why the problem - we did exactly that before this check-in - we just weren't exposing the canonicalized paths as part of what was being returned from the method.

[lajones]

Have updated it so the class NamespaceAndOutputPaths is internal and so is the method that uses it. Also found that 2 tests (including the one we're talking about here) should now be in EntityFramework.Commands.Tests to match their source code - so pulled them into there at the same time.

[natemcmaster]

Discussed in person with @lajones. We will keep the function and it's datatype internal.

[lajones]

Checked in with commit 036140f.