Add support for readme rendering

[Atreyagaurav]

Fixes #859

Suggestions are welcome, I'm only implemented the basic render for now.

Other things that might be needed:

• user flag to turn on/off readme rendering
• render html bits inside markdown (might to be fixed from markdown crate)
• other formats besides .md (.txt and , maybe .org)

[svenstaro]

I like the idea in general but it should fit into the general current design of miniserve. We already supported an --index file and perhaps this could be treated as an index file that still needs to be rendered? What do you think?

[svenstaro]

Btw quickest review ever. 😎

[svenstaro]

You can mark the PR as draft, then you don't need a [WIP] prefix.

[Atreyagaurav]

@svenstaro Thanks I didn't know draft was public.

As for index.html, html writing is complicated compared to markdown, hence I didn't check the index file.

Do we have to write the index file with all the paths ourselves, or just the other parts and the file/directories will be the same? If we can only use the readme part and keep the other contents same as default we can add a bool flag to say the index file is a markdown.

I think we can just add a separate add markdown render without less effort (relatively) and it'll be consistent design with how we share codes in other mediums I think it's worth looking into. Instead of generating a whole HTML. (Of course I'm talking here without knowing how index files work)

And we can improve it further. I am just done implementing a flag so it won't be active by default. And I'll look into better renderer.

[svenstaro]

As for index.html, html writing is complicated compared to markdown, hence I didn't check the index file.

I'm not saying we should actually write any additional HTML here. In fact, we most definitely shouldn't create any files and suprise users.

Do we have to write the index file with all the paths ourselves, or just the other parts and the file/directories will be the same? If we can only use the readme part and keep the other contents same as default we can add a bool flag to say the index file is a markdown.

Sorry, I do not understand this at all. Why would we write any additional HTML files? I meant earlier that we should try to combine the --index feature and the markdown rendering to create some kind of combined convergent solution here for miniserve as opposed to creating a divergent one (where we end up with two things that kind of do the same but use different flags and semantics).

I think we can just add a separate add markdown render without less effort (relatively) and it'll be consistent design with how we share codes in other mediums I think it's worth looking into. Instead of generating a whole HTML. (Of course I'm talking here without knowing how index files work)

And we can improve it further. I am just done implementing a flag so it won't be active by default. And I'll look into better renderer.

Perhaps it might make sense at this point if you toy around with the existing index functionality a little bit to check out whether this would fit with the markdown design somehow?

[Atreyagaurav]

If you think we need to integrate it with index flag then I'll look into how index is rendered and trace it. It'll take time, I'm not that good with html.

I'll add some logics to make sure when --readme flag is there --index should also be given/ or defaults to readme.md.

I still have some changes that I need to push like actually implementing the --readme flag, so I'll commit those and if I'm satisfied I'll change this from draft.

EDIT: And really thank you for quick responses, you saved my time. :)

[Atreyagaurav]

Do we have to write the index file with all the paths ourselves, or just the other parts and the file/directories will be the same? If we can only use the readme part and keep the other contents same as default we can add a bool flag to say the index file is a markdown.

Sorry, I do not understand this at all. Why would we write any additional HTML files? I meant earlier that we should try to combine the --index feature and the markdown rendering to create some kind of combined convergent solution here for miniserve as opposed to creating a divergent one (where we end up with two things that kind of do the same but use different flags and semantics).

I meant does the user need to write the entirety of the contents on the index file or will the directory listing part will be automatically generated? If it's the later then we can add readme with the same method, but if user has to write the whole index file, then I think it's better to separate that with the readme functionality where we only add the readme→html part to our template.

I looked into the index file, and looks like it completely replaces the directory listing, and if we go with using readme as a index file that needs to be converted then we'll have to add the directory listing to it using the same method anyway (i.e. generate html for directory listing, and then generate html from readme).

So I think it's better to keep them separate after all. It also matches the file's themes, index file is a way to index directory, and readme is just extra information about that directory. And README can appear sparsely, some directories might have it some might not. And irrespective of its presence, the directory listing will always be there.

Something like this:

A
├── B
│   └── README.md
├── C
└── D
    └── README.md

So even if there is no README in root, if you are visiting A/B then the README will be rendered.

if you provide index-file: it'll be used and directory listing will be disabled (whatever current default is)
if you have readme flag: default directory listing will have readme contents if there is a file named readme.md in the currently visiting directory. It'll be ignored if you have index file.

Current features of readme flag:

• Renders README.md after the directory listing
• Renders README.md if it's present in any nested path -- that you're currently visiting.

[Atreyagaurav]

@svenstaro Any thoughts? My feeling as I said before is readme feature added into current render is ok as index file is supposed to be entry point and readme isn't. Those two seems like different things to me.

[svenstaro]

Ok, I thought about this. Sorry, took some time. I want to make sure miniserve doesn't become a hodgepodge of features that don't really make sense.

I like the feature the way you proposed it and I agree it should be a completely separate mechanism from how the index is rendered. A few points:

• Should it conflict with index? If index is also provided, what should happen? Should index always prevail? I suppose it will, because it will also prevail over simple directory listing already but I'd like to align with you here.
• You write "Renders README.md if it's present in any nested path -- that you're currently visiting.". I'm not sure that makes sense or if understand it correctly. I'd expect a README to only be picked up in the current directory that we're in but not in any subpaths of our currently visited directory, that would cause great confusion I think (and it would also be expensive to find all READMEs potentially).

[Atreyagaurav]

Should it conflict with index? If index is also provided, what should happen? Should index always prevail? I suppose it will, because it will also prevail over simple directory listing already but I'd like to align with you here.

I think --readme doesn't matter when --index is provided, as once there is index file, then it'll be served and no directory listing will be made by miniserve. Considering readme is feature on the directory listing, it'll be ignored same way -D flag is ignored, since those are silently ignored we can do the same here?

You write "Renders README.md if it's present in any nested path -- that you're currently visiting.". I'm not sure that makes sense or if understand it correctly. I'd expect a README to only be picked up in the current directory that we're in but not in any subpaths of our currently visited directory, that would cause great confusion I think (and it would also be expensive to find all READMEs potentially).

Sorry. I wanted to say in case of nested paths we can render different README.md files automatically, in contrast to having to give the path of index file at start. Of course the one that'll be rendered at any moment is the one from current directory.

Also about the test, there is problem from clippy due to too many arguments in the page function, do you mind taking a look at it? I added readme argument in that page function that exceeded the limit, I don't know if we can modify other arguments to reduce the number, so you might have better idea if we can reduce it, or maybe increase the limit for clippy. When I built it in arch, it was showing only as warning and not error and the exit code of process was 0, so I didn't do anything but looks like it failed in nightly on ubuntu latest.

[Atreyagaurav]

Does it cancel automatically once one of the tests failed? Would have been nice to know if there are some errors so we could fix in one go. Anyway, as I've said previously I'm waiting for your opinion on what to do about that too many argument problem.

[svenstaro]

Does it cancel automatically once one of the tests failed? Would have been nice to know if there are some errors so we could fix in one go. Anyway, as I've said previously I'm waiting for your opinion on what to do about that too many argument problem.

Do this for now:

#[allow(clippy::too_many_arguments)]

I think we'll just ignore this as we know what we're doing here.

[svenstaro]

Also please rebase your branch so that the merge into your branch goes away as otherwise the history looks kinda funky.

[Atreyagaurav]

Looks like the formatting issue was because I hadn't set my editor to use spaces so there was tabs and it looked ok to me. I've made the other changes and force pushed. Sorry if there is some problem in intermediate commits as I'm not that familiar with rebasing.

As for the file opening issue, We can make a struct with readme name and the content and pass that instead, and read contents in the listings instead of rendering so we have it under same variable instead of two. I read it from rendering originally to just use one variable.

[Atreyagaurav]

@svenstaro I tried with Readme struct but haben't merged to master: Atreyagaurav@04ec70e

Is this good?

Edit: I merged it, so that it'll be incorporated here. Looks ok to me, and cargo build/fmt/clippy ran fine.

[Atreyagaurav]

My mind isn't adjusted to rust yet, sorry about that bool use in the struct. I've made the use of Readme struct better, I think. Making a blank struct when I could pass None was pretty stupid.

Let me know what you think. This should at least move the heavy processing (IO) away from rendering for now.

[Atreyagaurav]

Sorry didn't notice the spelling, I somehow only saw the README.md part, anyway it's fixed now. The .as_ref().unwrap() is just rust I guess, To make things safe. I did it as suggested by the compiler.

[svenstaro]

Merging as is. Thanks, this is great!

[svenstaro]

Oh, I just noticed we don't actually have a test for this. Would you like to add one after the fact, @Atreyagaurav?

[Atreyagaurav]

I've never written rust tests, and currently have no idea how to test readme thing, let me read other tests and see what I can do. Might take a week or so.

[svenstaro]

Not to worry! I think the tests are actually fairly easy to read and it might be a good learning exercise at any rate.