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

Enhancement: in docs move global options to a separate file #1852

Closed
waldekmastykarz opened this issue Oct 10, 2020 · 21 comments
Closed

Enhancement: in docs move global options to a separate file #1852

waldekmastykarz opened this issue Oct 10, 2020 · 21 comments
Assignees
Milestone

Comments

@waldekmastykarz
Copy link
Member

Recently, we have changed the CLI to always use docs from .md files. This lowers the effort in writing docs and simplifies the maintenance since we have a single version of docs for each command. We could simplify things further by extracting global options, which are the same for all commands, and storing them in a single file so that we don't need to repeat them in every file and can maintain them consistently in a single place for all commands.

For this to work, we need to check two things:

  1. Can we include the markdown-include plugin in our CI/CD pipeline?
  2. Would it be possible to extend markshell so that we can render information from referenced files in the terminal as well? /cc: @StfBauer
@garrytrinder
Copy link
Member

I'm in favour of this enhancement.

We may want to look at https://github.com/RedisLabs/mkdocs-include as well.

@StfBauer
Copy link
Contributor

StfBauer commented Oct 12, 2020

Not familiar with this but let me check.
Overall yes.

@waldekmastykarz
Copy link
Member Author

Good find @garrytrinder. Let's give it a try and see if we can get it to work.

@StfBauer
Copy link
Contributor

StfBauer commented Oct 12, 2020

@waldekmastykarz @garrytrinder

markshell@1.3.0-alpha0 should now support the externals.

ATM: I only implemented this form

Source file
# Heading Level 1 of main file

{!included_file.md!}

## Heading Level 2 of main file

{!included_file.md!}

I can add the other version too fi needed. If you really favour the other syntax I need to update the regex.

@waldekmastykarz
Copy link
Member Author

Awesome! Thank you for such a quick turnaround. We'll give it a go asap 🚀

@waldekmastykarz
Copy link
Member Author

It seems that we can use the syntax of the PyMdown snippets extension, which is already available in our infrastructure:

--8<-- "docs/cmd/_global.md"

@StfBauer it would be awesome if you could add support for this import notation in markshell. The path is tricky as it's relative to where the mkdocs.yml is located. I think it would be the easiest to expose this path as a markshell setting so that you don't need to worry about detecting the path in markshell yourself, but instead have folks specify it themselves explicitly through docsBasePath or something similar 🙂

As soon as this is supported in markshell, we can go an update our docs 💪

@StfBauer
Copy link
Contributor

StfBauer commented Oct 22, 2020

OMG @waldekmastykarz that is pretty tricky cause I don't have start and end for this definition. It is a hell of a syntax - might node-gyp help.

I take a look.

@waldekmastykarz
Copy link
Member Author

How about we'd start with supporting just one line with a regex, where the line must end with the quote, something like ^-{2,}8<-{2,}\s"([^"]+)"$

@StfBauer
Copy link
Contributor

Yeah that is no bigggy for my ;)

@StfBauer
Copy link
Contributor

StfBauer commented Oct 27, 2020

Ok so I changed the code and after this comment I will publish the new version of the package.

{! helloworld.md !}
--8<-- helloworld.md

Both syntax should include the external files. You never know. 🤷‍♂️

image

Thank you for the head start with the regex I had to modify it a bit and should now be able to include any other syntax with a minimum effort.

Types are also updated because there is a setting as requested that will be for now used in the PyMDown exclusively.

theme.includePath = path.join(__dirname, '../samples/');

Current version 1.3.1 with that external support.

Ladies and gents, update your engines.

@waldekmastykarz
Copy link
Member Author

Hero! I'll give it a try asap. Thank you very much! 👏

@waldekmastykarz
Copy link
Member Author

waldekmastykarz commented Oct 27, 2020

@StfBauer after upgrading to markshell@1.3.1, here is what I've found out:

  1. If I don't specify includePath, help is not being rendered at all
  2. The regex that detects filename is incorrect because it includes quotes in the filename, so the final path is like: /cli-microsoft365/docs/"docs/cmd/_global.md"

FWIW: I'm testing it on this branch https://github.com/waldekmastykarz/cli-microsoft365/tree/external-global-docs

@StfBauer
Copy link
Contributor

@waldekmastykarz 🤦‍♂️

  1. If you don't have your config correct, why should you get a content !?!?!?
    No seriously I have returned anything

  2. Regex can be fun - I found the g and removed it
    Should also work now.

1.3.2 looks more promising now

image

I hope:
image

I am just wondering about the docs/docs but I guess this is something you will fix right?

@waldekmastykarz
Copy link
Member Author

docs/docs is just our path. docs is required name for a subfolder by MkDocs and the first docs denotes where we have docs (as opposed to src or dist). If we wanted to get rid of the first docs, we'd need to put mkdocs.yml in the root, which is kind of odd.

As for not specifying the includePath, the reason I brought it up is because it seems like something optional. If you don't want to use it, you shouldn't have to specify it, right? 🙂

@StfBauer
Copy link
Contributor

Exactly and it is only used now for PyMDown. The only thing I could do to have the same treatment like the other plugin, when not specified. Not sure.

Case 1: not specified

let baseDir = path.dirname(filepath); // file of markdown file

Case 2: specified

let baseDir = this._theme.includePath // file in config

@waldekmastykarz
Copy link
Member Author

OK, so perhaps what was breaking the help is the fact that we had PyMDown include markers but were missing the necessary configuration. What would be helpful in such case to return a clearer error message what's wrong. Not rendering the whole help altogether is an indication that something is wrong somewhere but it's not very clear what's wrong exactly. 😊

@StfBauer
Copy link
Contributor

What are you thinking @waldekmastykarz? I could stack all errors instead of raising the errors and put them up front or at the end of the content? Would that help?

@waldekmastykarz
Copy link
Member Author

Absolutely! As long as they're displayed, it'll help a lot ❤️

@StfBauer
Copy link
Contributor

OK if you insist 👍

@waldekmastykarz
Copy link
Member Author

Including contents from external files works like a charm. I'll wait with submitting a PR until we sorted out StfBauer/markshell#23 so that we can do them in one go.

@waldekmastykarz
Copy link
Member Author

Issue confirmed fixed in markshell@1.3.7. We can proceed with this issue. Thanks @StfBauer ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants