Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional 'drafts' level to inclusion #3476

Merged
merged 6 commits into from Dec 11, 2023
Merged

Conversation

athackst
Copy link
Contributor

@athackst athackst commented Nov 20, 2023

This change adds a "drafts" configuration option so that files can be appropriately marked as 'draft' to be included in the local server, and files that shouldn't be included at all are marked in 'exclude_docs'.

This change helps #3450 by allowing users to fully exclude paths.

@athackst athackst force-pushed the feat/drafts branch 2 times, most recently from 9493d55 to 78a2c81 Compare November 20, 2023 06:07
@oprypin oprypin added this to the 1.6.0 milestone Nov 20, 2023
@oprypin
Copy link
Contributor

oprypin commented Nov 20, 2023

Thanks! I wanted to do just this, again thanks for pushing this.
I'll of course add my nitpicks 😅
And of course this is a breaking change (of a feature that was only in one release) but it's for the best.

@athackst
Copy link
Contributor Author

Sure, pick away.

I know it's a breaking change, but it shouldn't be too bad for users.

I think the addition of a "drafts" level makes it easier to understand the intention and behavior.

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I'll probably move forward with this.

docs/user-guide/configuration.md Show resolved Hide resolved
mkdocs/tests/build_tests.py Outdated Show resolved Hide resolved
mkdocs/structure/files.py Outdated Show resolved Hide resolved
@athackst
Copy link
Contributor Author

Another option might be to deprecate the 'exclude_docs' setting and rename what 'exclude_docs' is doing in this PR to 'ignore'?

That way folks using 'exclude_docs' will get a warning.

@oprypin
Copy link
Contributor

oprypin commented Nov 22, 2023

@athackst
Wow that's actually even better. Great idea! And yes, eventually removing the drafts functionality would be for the best. I'm still thinking on this and a bit low on time in the last days.
So, new ignore setting, deprecate exclude_docs. That sounds best so far.

@athackst
Copy link
Contributor Author

@oprypin
Not quite - I updated the code to what I was thinking. Deprecate exclude_docs and new ignore and drafts settings.

I think the drafts functionality is good to keep.

The issue is that there isn't an additional way to ignore files in the docs_dir, so files we wanted to ignore were included in serve. I think this change appropriately decouples those concepts and opens the door to a couple of new features you might consider in the future:

  1. Unblocking setting docs_dir from the root directory Support docs from the root directory. #3450
  2. Supporting a command line flag to include drafts or not on serve (by updating the is_in_serve function based on this flag)
  3. Supporting a .mkdocsignore type file to locally specify additional ignore patterns (I do this in my plugin and have found it useful)

@athackst
Copy link
Contributor Author

athackst commented Nov 28, 2023

Let me know if you have any more nit picks!

I could "move" the exclude_docs option to either ignore or draft. I wasn't sure which use case was more popular, so I just went full deprecation so folks would be aware of a breaking change.

@oprypin
Copy link
Contributor

oprypin commented Dec 9, 2023

Here are my current thoughts:


I had a quick look around in search, and it appears that almost every use of exclude_docs is as the "ignore" feature, not the "draft" feature. It has actually gotten a bit popular.

With that in mind, I think the cumulative annoyance of renaming this option will be greater than the rare annoyance that the feature suddenly stopped doing drafts.

With that, I propose to move back to the plan of not deprecating the name exclude_docs.


Then, what to do about drafts... Actually I was hoping to remove it entirely and instead immediately provide a plugin to do the same feature. But actually for a plugin, to provide all the same functionality of still validating links, its source code would have to be 5x clunkier, if at all possible.

There also would be the matter of having two breakages for this feature - first renaming it, then dropping it.

So perhaps keep this feature permanently? Maybe name it draft_docs and not drafts?

@oprypin
Copy link
Contributor

oprypin commented Dec 9, 2023

So I force-pushed the exact old version of your commit into this branch instead.

@oprypin
Copy link
Contributor

oprypin commented Dec 9, 2023

(And could we just use merge instead of rebase+force as the workflow)

@oprypin
Copy link
Contributor

oprypin commented Dec 9, 2023

I have pushed changes for final touches per my preference.
I think this is ready, I will not change my mind on the plan anymore 😅
But further comments still welcome for sure

@athackst
Copy link
Contributor Author

athackst commented Dec 9, 2023

Thanks for the discussion, it's been a pleasure. I'll update my other PR after this one merges.

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you as well, and thanks for the patience

@oprypin oprypin merged commit 3e809b6 into mkdocs:master Dec 11, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants