Closed diegomarangoni closed 2 years ago
can I have a review here? maybe @bih?
Starting to review
Hello, @diegomarangoni thanks for contributing 🎉 Added only one comment to the test file! Can you take a look at the failed checks, please?
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 ? Please let me know though @diegomarangoni if I have misunderstood the problem that you are trying to solve and your attempt to solve it.
@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 ? Please let me know though @diegomarangoni if I have misunderstood the problem that you are trying to solve and your attempt to solve it.
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 `^[a-zA-Z0-9-/]+$`."_
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 site_name
instead of being used as page title, will be used as the navigation path.
Considering a scenario where the documentation is designed to have its own dedicated page, with site_name
being a human-friendly title, embedding this doc would not be possible.
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
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:
myapp-backend
: it has his own mkdocs file, his own mkdocs serve
page, and the site_name
being MyApp Backend service
myapp-frontend
: like the previous one, but I don't have control over this repository and site_name
being MyApp Frontend application
myapp-docs
: new repository created in order to centralize backend
and frontend
docs in a single placeSo the problem here is that I can't easily use this plugin without having to change both frontend
and backend
site_name
first and at the cost of having less friendly titles on their mkdocs serve
page
Here is how the page looks like:
.
Thanks for the contribution @diegomarangoni! Shipping this now as a patch release. Will look toward the .yaml
support PR as a minor version update after this.
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.