-
Notifications
You must be signed in to change notification settings - Fork 75
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
Automatically slugify site_name #58
Automatically slugify site_name #58
Conversation
can I have a review here? maybe @bih? |
Starting to review |
__tests__/integration/test.bats
Outdated
@@ -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 |
There was a problem hiding this comment.
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:
- Issue: Broken version 0.4.9 #23
- Pull request: use os.path.join instead of convert '/' to '-' #24
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Users will lose favorite links that point to the current path style
- 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:
If you try to access the page using the previous URL will see a not found page:
And if you don't update the link in your markdown, they will be broken links pointing to the old URL (not found pages):
To fix this, users will need to update the paths in their doc files:
There was a problem hiding this comment.
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
Hello, @diegomarangoni thanks for contributing 🎉 |
hey @camilaibs, thanks for the review I did some changes, it should works now |
@diegomarangoni - I have been thinking about this a bit. We are a bit reluctant to introduce an automatic change to a user's site_name (assuming they have used a character that doesn't pass the regex). I think the favoured solution would be to inform the user that their site_name includes an invalid character - and then give a good error message that guides them in the right direction. And I think we have that in place, right @camilaibs ? |
Hey @garyniemen, the message we have today is: "Site name can only contain letters, numbers, underscores, hyphens and forward-slashes. The regular expression we test against is |
hey @garyniemen, let me better explain the use case this PR is intended to solve. Currently, this plugin requires that every included documentation must be designed to be embedded, meaning the Considering a scenario where the documentation is designed to have its own dedicated page, with So this will require changing the documentation title to something less user-friendly and also assumes that the user has control over this project, which might not be the case. Let me know if you have any question |
__tests__/integration/test.bats
Outdated
@@ -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 |
There was a problem hiding this comment.
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
Hi again @diegomarangoni - sorry for the delay in replying. I am still a bit confused. Is this use case specifically for a Monorepo? Assuming so, and within that context, can you describe what the 2 use cases are that you are trying to simultaneously address. Will the change that you are making fix one of these use cases - but still keep the other working fine? And - what is the relation between these 2 use cases and the previous discussion around Regex? |
hey @garyniemen, the use case I describe is for multiple repositories, let me bring an example that might help: I have 3 repositories:
So the problem here is that I can't easily use this plugin without having to change both |
Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
Thanks for the contribution @diegomarangoni! Shipping this now as a patch release. Will look toward the |
Currently, this plugin fails if an embedded mkdocs
site_name
field does not match the regex^[a-zA-Z0-9_\-/]+$
.This PR change this behavior to automatically generate a slug based on
site_name
, this will allow to maintain user-friendly names and still be able to embed.