search

Doctave / doctave

A batteries-included developer documentation site generator
https://cli.doctave.com
MIT License
565 stars 32 forks source link

Make doc root configurable #21

Closed datdenkikniet closed 2 years ago

datdenkikniet commented 3 years ago

See title.

This simply starts using the facilities that were already in place to configure the documentation root.

Expand doctave init so that you can select which directory should be used as document root

Fixes #20

Unsure what/if there is anything that can be tested here, since all that really changes is that this path is now read from the configuration file instead of using the default docs/

begleynk commented 2 years ago

Thanks for taking the time to implement this (and so cleanly too)! This feature had been on the todo list for a while.

One thing that jumped at me was if we should use doctave init <docs dir> or doctave init --docs-dir <docs dir>. The reason I'd prefer the second is that you could imagine init taking as it's first argument the directory of the project that you want to document (where the doctave.yaml gets placed). The docs dir is sort of a secondary configuration, so I feel it should be a flag instead of the main positional argument to the command.

Would you be willing to make that change to your PR?

As for testing, we should probably include an integration test for this. One example to look at is this test that first runs the init command, followed by build just to check that the site gets generated. A similar test with the --docs-dir flag should be added.

begleynk commented 2 years ago

Also, could you rebase your changes on top of master? I realised my GitHub actions config wasn't running tests for pull requests. That change should be picked up if you include this commit (3e1fd1d)

datdenkikniet commented 2 years ago

Yes, that is a good point. Now that you mention it, the 2nd argument should definitely be the where you want to put your doctave.yaml, not where the docs are, since that's how almost every other project does it too. I'll make this change in a few hours.

Will rebase, too!

datdenkikniet commented 2 years ago

I also just noticed that there are a few fmt-only changes in my commits (as a result of running cargo-fmt automatically), for instance this one: image

Are those OK to keep in?

datdenkikniet commented 2 years ago

Okay, I think I've fixed most of the things now.

Renamed doc_root to docs_dir everywhere for more consistency, and made it an option instead of an optional positional argument.

Added integration tests to verify that init with docs-dir specified creates the correct files, doesn't create files if the custom directory already existed.

Added an integration test that verifies if serve respects the docs_dir setting for good measure.

As far as I can tell, all of the other code that is tested depends uses the docs_dir field in in Config, meaning that those code paths are already tested.

begleynk commented 2 years ago

Ah that test failure is something I've seen before - I haven't come up with a good solution for it yet. Basically it's running the tests in parallel and things break due to the ports colliding. It's timing dependent which makes it frustrating. Maybe let's skip the serve integration test for now, as long as we've manually verified the change. I need to refactor those types of tests.

I also just noticed that there are a few fmt-only changes in my commits (as a result of running cargo-fmt automatically), for instance this one: image

Are those OK to keep in?

This is fine. I think this is probably a cargo fmt version issue? I can deal with adding a linting step that standardizes this later. Just include those changes too for now.

datdenkikniet commented 2 years ago

OK! I've removed the integration test now.

This is fine. I think this is probably a cargo fmt version issue? I can deal with adding a linting step that standardizes this later. Just include those changes too for now.

I'm unsure. I know that I'm running the default rustfmt style, so unless you're running a custom setup that shouldn't be necessary. If that is the case, then we should consider adding a rustfmt.toml to the root of the project. That'll make the linting step much easier to detect locally, and give rustfmt (which is directly called by cargo fmt) a nice configuration file to work with during the CI.

begleynk commented 2 years ago

Thanks for taking the time to work on this! This will be included in the next release, which is likely coming in a couple weeks.

Will have to look into this rustfmt issue a bit. Agreed that the project should set the standard.