Fix name clashes for auto-generated index files when using the same name for subdirectories

GeertvanHorrik commented 7 years ago

Fixes #57

FransBouma commented 7 years ago

It has some conflicts as I merged the previous update. The replace function can be smarter I think, see this SO question: https://stackoverflow.com/questions/3210393/how-do-i-remove-all-non-alphanumeric-characters-from-a-string-except-dash

FransBouma commented 7 years ago

I'd moved the logic in the property to create the name to a method as properties shouldn't be that big. This isn't a big deal.

What is a bit of a big deal is that this changes root pages' filenames, even if they didn't have a clash before. This is a problem as documentation that already has been generated with previous versions of DocNet will get new filenames for auto-generated index files (we use these in our docs at places). This breaks links to the original pages (we have these in our docs) and that's something I want to avoid, especially as this is a fix for a problem that only occurs in some situations.

So, sadly, the current situation can't change, the fix has to solve the problem without producing different files for the situation when there's no clash. It is a bug in the original code (the parent container's TargetUrl always results in "", so the filename is never prefixed with any kind of path), but one which we rely on ourselves, so abit unfortunate ;).

I don't know of any elegant solution, to be honest :/

GeertvanHorrik commented 7 years ago

You can always update your current pages to be redirects to the new ones.

FransBouma commented 7 years ago

There are many pages, and if I can avoid updating those (over 3 versions), I will. Not only that, but I can't fix external links to the docs, or other people's docs who use docnet and have docs generated by it.

I'm sure there's a solution for this, but the current PR represents a change that I can't make. It's not all lost, perhaps the code can be re-used for a solution to the problem that doesn't break existing documentation sets. We just have to think abit about what solution will work :)

GeertvanHorrik commented 7 years ago

Well, in theory your current docs should be considered broken. But let's assume they are not, then we could use flat files to have the same logic as before. Then we will only update auto-index files that are at least 1 level deep (but even then you'll have an issue where they might be broken).

What we could do is auto-generate a forwarder on the old files that are possible clashes. Then if a user accidentally uses an old one, they will be forwarded to the new (but if broken, it's still broken). If they navigate through the new trees though, they will always use the new system.

FransBouma commented 7 years ago

I disagree with your point that the current docs are broken, they're not, actually (they work fine). They will be broken when a documentation set is defined which results in duplicate files (due to the bug). If I keep things as they are now, documentation generated with it will work fine. The thing is that it won't generate proper files if this particular clashing situation is met, and for that situation we should find a solution. I find the forwarder solution not that great as off-line reading still won't work (and we ship the docs for offline reading).

Let's look at a solution which fixes the clashing situation and which keeps the current situation as well. I don't mind a setting in the docs which can be enabled to get the filenames as you have defined in the PR. The only problem with that is that the filenames are created at a spot where the config file isn't reachable.... :/

GeertvanHorrik commented 7 years ago

Offline reading will work just fine with forwarding like this:

<head>
    <meta http-equiv="refresh" content="0; url=./welcome/index.htm" />
</head>

FransBouma commented 7 years ago

I'm not going to merge the breaking change, sorry. I see that the redirect might work, but it's a solution for a problem users currently don't have (as the bug isn't common, I haven't run into it in our large sets of docs and haven't heard from others as well).

We'll have to think of something else.

GeertvanHorrik commented 7 years ago

But I don't understand. The old situation will keep working? Then what's the breaking change?

FransBouma commented 7 years ago

1) I have to generate the older files and maintain them too (and the code to do so!) 2) they'll always be there but only to redirect to a new page 3) I don't need all that now as things work fine for everyone, except for the few cases who run into the clashes.

Please understand, I don't want any of this, I want a solution for when the clash happens and only then a file which doesn't clash should be generated.

FransBouma commented 7 years ago

How about this:

1) by default, everything stays as it is 2) a settings is added (if not specified, it will use its default, which will give the behavior or 1)), which controls whether the auto-generated index.htm file is stored in the root (default) or in the folder of its container (which will solve the clashing)

This requires a fix for the bug where the path of the container is always "" at the moment and a setting which controls whether it will keep the value at "" or will use the path it should have.

I'll think about what causes the real issue and see if I can solve it that way, with the setting as a way to control existing and fixed behavior.

GeertvanHorrik commented 7 years ago

What if we generate this new structure only when clashes happen? We will need to expose this somehow and make it a 2-step generation process:

Determine paths
Generate all content (with the right target urls)

GeertvanHorrik commented 7 years ago

Ah, just noticed you also commented. A setting would work as well, but in your case instead of generating a longer file name, you will put it in the right folder. That would at least make the urls look much better than they do now, which would work as well in my case.

FransBouma commented 7 years ago

good :) I'll first investigate what's the cause of the bug, then see what can be done about it and whether it's more work to fix that or to do a full tree-first approach as you suggested. Will try that tomorrow :)

FransBouma commented 7 years ago

The bug is more subtle. It's not that everything is generated into the root, it's that there's a lack of a delimiter.

I have this json in my docnet.json file:

"Pages": 
{
    "__index": "index.md",
    "Welcome!": "docconventions.md",
    "What's new in v5.2": "whatsnew.md",
    "Migrating your code": "migratingcode.md",
    "LLBLGen Pro Runtime Framework features": "supportedfeatures.md",
    "Getting Started" :
    {
        "__index": "getting_started.md",
        "Quick start guides" :
        {
            "From Database to source code": 
            {
                "__index": "qsg\\dfscenario1\\index.md",
                "Create a project" : "qsg\\dfscenario1\\CreateProject.md",
                "Create entities based on a database": "qsg\\dfscenario1\\df_reverseengineer.md",
                "Generate source code":"qsg\\dfscenario1\\generatesourcecode.md"
            },
            "From nothing to database and source code":
            {
                "__index": "qsg\\mfscenario1\\index.md",
                "Create a project" : "qsg\\mfscenario1\\CreateProject.md",
                "Create an entity model":
                {
                    "Choice 1: Quick Model": "qsg\\mfscenario1\\AddEntityQuickModel.md",
                    "Choice 2: Manual adding through entity editor": "qsg\\mfscenario1\\AddEntityEntityEditor.md",
                },

The 'Create an entity model' section doesn't have an index page defined, so one will be generated automatically. It will be generated as: qsg/mfscenario1Createanentitymodel.htm. The rest of the files will be in mfscenario1: qsg/mfscenario1/AddEntityQuickModel.htm.

For some pages this will be they'll end up in a folder higher up than they're meant to be, which is the root (as the root always has an index page otherwise the tool stops).

The problem is here: https://github.com/FransBouma/DocNet/blob/master/src/DocNet/NavigationLevel.cs#L307, in the string.Format() call. It simply concats the path in front of the file, so the delimiter is lost.

Have to think about how to fix this properly w/o breaking anything.

FransBouma commented 7 years ago

Fixing this shows up some nasty edge cases the author of docs likely won't think of:

"Linq to LLBLGen Pro":
{
    "Getting started": "Using the generated code\\Linq\\gencode_linq_gettingstarted.md",
    "General usage": "Using the generated code\\Linq\\gencode_linq_generalusage.md",
    "Prefetch paths": "Using the generated code\\Linq\\gencode_linq_prefetchpaths.md",
    "Function mappings": "Using the generated code\\Linq\\gencode_linq_functionmappings.md",
    "Remarks and limitations": "Using the generated code\\Linq\\gencode_linq_remarkslimitations.md",
},

If I do the 'right thing' which is: Value = Path.Combine(Path.Combine(path??string.Empty, nameToUse), "index.md") (and not filtering the name from spaces etc.) so I'll get an index.htm in the folder of the container, I have to specify the files inside a section with the right folder. Above snippet shows this isn't the case: I get two folders: Using the generated code\Linq\ and Using the generated code\Linq to LLBLGen Pro\, the latter with the index.htm, as it uses the name of the container as the folder. So placing them a directory higher actually makes more sense: the folder names then never have a mismatch.

Using Value = Path.Combine(path??string.Empty, nameToUse + ".md") fixes the problem I showed in my previous post but doesn't solve the one in your clash repro docs. The reason there is that the containers have no path set, there's no documentation path defined for these. Therefore 'path' here is empty (because what value should it get?) So the change to add the delimiter doesn't solve your scenario, it solves one of them, in the case when the parent container does have a path.

so in your case, this solves it:

{
    "Name" : "DocNet repro documentation",
    "Source" : ".",
    "Destination" : "../",
    //"IncludeSource": "Includes",
    "Theme" : "Default",
    "ConvertLocalLinks" : true,
    //"SourceFoldersToCopy" : ["images", "includes"],
    "Footer" : "",
    "Pages" : 
    {
        "Somestuff 1" :
        {
            "__index": "somestuff1/index.md",
            "Introduction" : 
            {
                "Getting started" : "somestuff1/introduction/getting-started/page1.md",
            },
        },
        "Somestuff 2" :
        {
            "__index": "somestuff2/index.md",
            "Introduction" : 
            {
                "Getting started" : "somestuff2/introduction/getting-started/page2.md",
            },
        }
    }
}

This gives the parent "Somestuff 1" a folder, through its __index parameter. Also with the current code, as it will generate the foldername into the filename (so there will be 2 files: somestuff1Introduction.htm and somestuff2Introduction.htm)

So adding the delimiter fix makes the file move to a folder of the container, which breaks current docs, and won't fix all scenarios and thus isn't a proper solution either (besides, it's hard to make this configurable).

To fix #57 the parent container has to get an index so it has a folder, like described above. The issue isn't solve with that tho, it's still there, but sadly a side effect of how things are organized in the tool at this point. (best way would have been perhaps to assign folders to sections and all files in a section inherit the folder of the section, so there never would be a folder mismatch like I described earlier too).

I'll close this PR as it's not going to be merged. I'll add a new issue referencing to this PR and the posts above to document that it's an open, known issue and that a workaround exists.

GeertvanHorrik commented 7 years ago

Thanks for the research and detailed explanation. Since these are auto-generated docs (using the reference/**, there is no real workaround for me I guess). I understand that you don't want to merge this pr.

FransBouma commented 7 years ago

Please see https://github.com/FransBouma/DocNet/issues/64 as I think there is a solution, but it's more drastic than a filechange...

FransBouma / DocNet

Fix name clashes for auto-generated index files when using the same name for subdirectories #61