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.

Sign up for GitHub

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

Jump to bottom

Automatically slugify site_name #58

Merged
merged 9 commits into from
Oct 27, 2021
Merged

Automatically slugify site_name #58

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
57eba4e
change getAlias to convert site_name to a slug
diegomarangoni Oct 12, 2021
b98f9b6
Update unit tests to work with slugs
diegomarangoni Oct 12, 2021
3f728e5
Remove limitation
diegomarangoni Oct 12, 2021
61510dc
Bump plugin version
diegomarangoni Oct 12, 2021
d71cb03
Slugify only if didn't match regex
diegomarangoni Oct 19, 2021
37f7b33
Use slugify library compatible with python 3.5
diegomarangoni Oct 19, 2021
a1c96e3
Rename test fixtures.
iamEAP Oct 27, 2021
802f89a
Prepare for patch release.
iamEAP Oct 27, 2021
5c1af83
Merge branch 'master' into add-site-name-slugify
iamEAP Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions __tests__/integration/test.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ teardown() {
@test "builds a mkdocs site with site_name containing slash" {
cd ${fixturesDir}/ok-nested-site-name-contains-slash
assertSuccessMkdocs build
assertFileExists site/plugins/example-Folder/index.html
assertFileExists site/plugins-example-folder/index.html
Copy link
Contributor

@camilaibs camilaibs Oct 19, 2021 •
edited

Choose a reason for hiding this comment

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

Looks like a "/" should not be replaced by "-" in the path because apparently, it causes a not found error that has been solved in the past here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change a bit, I will slugify only if doesn't match the regex

Copy link
Contributor

@camilaibs camilaibs Oct 19, 2021 •
edited

Choose a reason for hiding this comment

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

if I use disallowed characters (like "@"), it compiles, but some pages are not found:

sample-docs/v3/mkdocs.yml
image

image

error

Copy link
Contributor Author

@diegomarangoni diegomarangoni Oct 19, 2021 •
edited

Choose a reason for hiding this comment

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

slugified site_name will naturally have different paths than the ones matching the regex
in this specific case, it will require to change the link reference to ./versions-v3/reference.md

Copy link
Contributor

Choose a reason for hiding this comment

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

So this will introduce significant changes, if we continue, it would be good to have instructions for this new path pattern. I'd like to hear from other reviewers just to make sure it's okay to proceed with this new standard.
What do you think @emmaindal, @iamEAP, @ottosichert and @OrkoHunter?

Copy link

@garyniemen garyniemen Oct 20, 2021 •
edited

Choose a reason for hiding this comment

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

Can somebody map out from a product perspective what the implications of this suggested change are?

Copy link
Contributor

@camilaibs camilaibs Oct 20, 2021 •
edited

Choose a reason for hiding this comment

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

Correct me @diegomarangoni, if am I wrong, please 🙏🏻

What will change is that we will replace all invalid characters with "-" in the site_name when generating documentation without throwing an error with instructions to not use those characters there.

The consequences are:

  1. Users will lose favorite links that point to the current path style
  2. And should also update links in their markdown files to use the new path pattern instead (the breaking change)

Example:

This is a new link path using "versions-v3" instead of "versions/v3:
slugify

If you try to access the page using the previous URL will see a not found page:

page not found

And if you don't update the link in your markdown, they will be broken links pointing to the old URL (not found pages):

wrong link

To fix this, users will need to update the paths in their doc files:

index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's correct, but there is one important difference:

if you don't change your site_name and continue to use the pattern matching regex (^[a-zA-Z0-9_\-/]+$), nothing changes

these consequences you mentioned only apply to cases where you decided to change your site_name to something that does not match the regex

[[ "$output" == *"This contains a sentence which only exists in the ok-nested-site-name-contains-slash/project-a fixture."* ]]
}

Expand All @@ -139,6 +139,11 @@ teardown() {
[[ "$output" == *"This contains a sentence which only exists in the ok-mkdocs-git-revision-date-localized-plugin/project-a fixture."* ]]
}

@test "builds a mkdocs even if !include path has site_name containing spaces" {
cd ${fixturesDir}/error-include-path-site-name-contains-space
Copy link
Member

Choose a reason for hiding this comment

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

Just one last update and I think this is good to merge. Could you update the name of this fixture to indicate that no error is expected? Maybe even just no-error-include-path-site-name-contains-space

assertSuccessMkdocs build
}

@test "fails if !include path is above current folder" {
cd ${fixturesDir}/error-include-path-is-parent
assertFailedMkdocs build
Expand All @@ -151,12 +156,6 @@ teardown() {
[[ "$output" == *"[mkdocs-monorepo] We currently do not support nested !include statements inside of Mkdocs."* ]]
}

@test "fails if !include path has site_name containing spaces" {
cd ${fixturesDir}/error-include-path-site-name-contains-space
assertFailedMkdocs build
[[ "$output" == *"[mkdocs-monorepo] Site name can only contain letters, numbers, underscores, hyphens and forward-slashes. The regular expression we test against is '^[a-zA-Z0-9_\-/]+$'."* ]]
}

@test "fails if !include paths contains duplicate site_name values" {
cd ${fixturesDir}/error-include-paths-duplicate-site-name
assertFailedMkdocs build
Expand Down
1 change: 0 additions & 1 deletion docs/limitations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# Caveats / Known Design Decisions

- In an included `mkdocs.yml`, you cannot have `!include`. It is only supported in the root `mkdocs.yml`
- In an included `mkdocs.yml`, your `site_name` must adhere follow the regular expression: `^[a-zA-Z0-9_\-/]+$`
13 changes: 2 additions & 11 deletions mkdocs_monorepo_plugin/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
import logging
import os
import copy
import re

from slugify import slugify
from mkdocs.utils import yaml_load, warning_filter, dirname_to_title, get_markdown_title
log = logging.getLogger(__name__)
log.addFilter(warning_filter)
Expand Down Expand Up @@ -205,16 +205,7 @@ def getDocsDir(self):
return self.navYaml.get("docs_dir", "docs")

def getAlias(self):
alias = self.navYaml["site_name"]
regex = '^[a-zA-Z0-9_\-/]+$' # noqa: W605

if re.match(regex, alias) is None:
log.critical(
"[mkdocs-monorepo] Site name can only contain letters, numbers, underscores, hyphens and forward-slashes. " +
"The regular expression we test against is '{}'.".format(regex))
raise SystemExit(1)

return alias
return slugify(self.navYaml["site_name"])

def getNav(self):
return self._prependAliasToNavLinks(self.getAlias(), self.navYaml["nav"])
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ mkdocs>=1.1.1
mkdocs-material>=5.1.0
mkdocs-git-authors-plugin==0.3.2
mkdocs-git-revision-date-localized-plugin==0.5.0
python-slugify==5.0.2
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

setuptools.setup(
name='mkdocs-monorepo-plugin',
version='0.4.16',
version='0.4.17',
description='Plugin for adding monorepository support in Mkdocs.',
long_description="""
This introduces support for the !include syntax in mkdocs.yml, allowing you to import additional Mkdocs navigation.
Expand Down