fix folder already rendered with same name

[enricosada]

fix #246

[latkin]

Our internal build is still on nunit 2.2.2, so TestCaseSource is not available.

[enricosada]

i dont understand, internal ci doesn't run the repo bat files for tests?

[latkin]

Internally, source is in big TFS repo along with other Visual Studio stuff, and has various fixed sets of dependencies. Projects used to build shipping VF# tools are not the same as in this repo (though close), tests are run through different systems. We are looking at unifying/simplifying, but for now NUnit stuff needs to be compatible with 2.2.2.

[latkin]

Thanks for the PR, this will be great to have fixed. Looks like a good set of tests.

Overall I'd just recommend adding some comments and refactorings so that it's more clear how exactly this gets validated, and what's considered valid/invalid.

[enricosada]

thx for review @latkin , i'll send an update

[enricosada]

Sry for delay.
During refactoring i tried to fix root problem (add multiple nodes with same name) but is not ready yet.
I'll complete this one and update it, max this weekend, and send the other fix as separate pr

[enricosada]

Thx for Ping :)

[KevinRansom]

Enrico, is there any progress to report on this?

Thank you

Kevin

[enricosada]

i am near to end, i used lot of time for complete but is going nice.
i need to update status more while working

[KevinRansom]

What you do is fine mate, and thank you so much for taking on this work.

Kevin

[yupferris]

status? :)

[latkin]

@enricosada I've heard before that this bug is annoying, it will be great to have your fix. Do you have an ETA on an update?

[enricosada]

i added the new version.
i'll check why build failed, but i rebased this branch onto master HEAD before push

the idea is to used a tree structure (the RoseTree) and domain types, and check the tree for the invalid folder logic or others (like remove unused folders)
The function also remove unused msbuild <Folder ..> items

Every node is a ProjectTreeNode (a node in solution explorer):

• Folder, the virtual folder used by solution explorer
• Item, an item with a reference to the msbuild item

every node can have children, exactly like solution explorer, so there is not a 1-1 mapping with msbuild items

the flow is:

• explode every msbuild item as tree
• add every tree to the project root
• discard unused folders
• check for invalid folders

The algorithm use domain types, so we can unit test, instead of run the integration test with full visual studio (slow).
I think this is good because we can start take advantage of f# discriminated union for msbuild nodes (like ComReference etc) and behaviours

[enricosada]

btw, can i add the FsCheck as reference from nuget?
For vs extension tests (not core, so np for profiles)

sometime is easier to use that for tests (like path, tree, list) because easy invariant

[KevinRansom]

I like this, it is a big ambitious fix. We will have to go over the code more thoroughly but it certainly looks, nice with a lot of detailed test cases, thank you for this.

However, given where we are in the cycle and how late it is, I think we are going to have to postpone taking this until the next version. We are quite thoroughly locked down right now, and accepting changes only with the minimum necessary footprint changes. It looks like a bunch of opportunistic and good restructuring was done, but we do not really have the runway to ensure that such a big set of changes to our project system do not have any impactful side effects.

It is a shame, I was hoping that we would ship Dev 14 with a fix for the issue. We will not decline this pull just keep it until we can accept it into F# V.Next. People should continue to review and thoroughly vet this submission.

[msftclas]

@enricosada, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[latkin]

@enricosada is this big change still beneficial/needed, given that the original bug was fixed?

[latkin]

Closing this out as original bug has been fixed. Please migrate this work to the tip of OOB and re-open if you'd like to revisit this for an upcoming release.