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

doc, tools: refactor html generation #6974

Closed
wants to merge 17 commits into from
Closed

doc, tools: refactor html generation #6974

wants to merge 17 commits into from

Conversation

eljefedelrodeodeljefe
Copy link
Contributor

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc, tools

Description of change

I was picking up the guides generation from #5408. While doing this I moved forward without using another external dependency. To re-use the html-path entirely wouldn't have been possible. So I refactored this to be more modular in style, and will continue to just introduce "target flags" guide < > html and branching components accordingly.

Also a rather critical thing is, that up until this PR HTML generation was done via piping over stdout. I have refactored this to make a call to fs.writeFile(). This also leads to a increase possibly.

cc @nodejs/documentation

@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 25, 2016
@addaleax
Copy link
Member

Quick heads up: This is likely going to conflict with #6690 and #6943, so be prepared to rebase 😉

@eljefedelrodeodeljefe
Copy link
Contributor Author

That's fine. I came last. Do you wanna go ahead merging yours?

@addaleax
Copy link
Member

@eljefedelrodeodeljefe I was planning on waiting at least the mandatory time period. ;)

@eljefedelrodeodeljefe
Copy link
Contributor Author

Alright. Gonna wait too then :)

console.log(html);
toHTML(input, inputFile, template, nodeVersion, (er, html) => {
if (er) throw er;
const filename = `./out/doc/api/${path.basename(inputFile, 'md')}html`;
Copy link
Member

Choose a reason for hiding this comment

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

I am not a 100 % sure I like having the output paths hardcoded here:

  • Generally, the doctool can be run on any .md file and is somewhat agnostic about the project it belogs to (i.e. it doesn’t even have to be Node.js docs that are being processed), and I’d prefer to keep it more or less that way
  • This would make it harder to include docs from other directories if that’s desired at some point (doc/topics being a likely candidate?)
  • Having the Makefile output include the output file makes it easier to reason about what is actually happening during building

So… what are the advantages of doing it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. it can with that. basically you define the behaviour of the guide, html etc. rendering within that switch case. It would of course be possible to have a flag for the output in the makefile. A flag okay, having unix control flow through piping not so okay - especially since we have all the necessary tools o.board. So +1 for modularity through, flags, configs etc. We would need to decide what it should be though. Currently and previously the configuration basically happened in the switch case.
  2. as noted above, it makes it even easier. We can improve though.
  3. The makefile around there is more verbose than having it in JS, since it only consist of variables that will be iterated over files, which is probably baggage from the early days. There is no real reason not to do in JS to gain more flexibility and maintainability.
  4. There is no apparent advantage but also there is no real change in there. The advantage will be that I can branch for 'guides' and use components 'html' doesn't need. Namely We will need a custom component for the _toc since it will be nested.

@addaleax
Copy link
Member

To clarify… the changes here currently consist of:

  • Moving code around
  • Writing to a output file whose path is computed from the input file
  • Adding a _toc.md in doc/guides/

… right?

@eljefedelrodeodeljefe
Copy link
Contributor Author

  • Moving code around
  • Writing to a output file whose path is computed from the input file
  • Adding a _toc.md in doc/guides/
  • yes.
  • whose path can be set and/or be computed from inputs or configurations.
  • adding the _toc and switch on/off components that wont be needed for guides or mutually 'html'

@eljefedelrodeodeljefe
Copy link
Contributor Author

@addaleax now diving into the doctool I actually regard https://github.com/nodejs/node/blob/master/Makefile#L260-L289 to be really bulky and we could draw all of it into a docbuild.js and hence make the doctool even more of a standalone module. Currently it depends on the wildcard-part, imo.

@addaleax
Copy link
Member

@eljefedelrodeodeljefe Keep in mind that the current situation enables use of makes capabilities such as checking modification times, which I do find pretty helpful when it editing the docs themselves

Also, I don’t see any disadvantage in using output redirection (which neither uses pipes nor is really unix-specific, btw), but maybe I’m just not fully understanding what you are getting at.

@eljefedelrodeodeljefe
Copy link
Contributor Author

I think I am gonna include the bigger change for the guides here. Then it should get clearer.

About makes cap, well okay. For the docs I don't see the benefit too much.

@eljefedelrodeodeljefe
Copy link
Contributor Author

@addaleax I think I only can resolve this when merging. I have manually rebased it now at eljefedelrodeodeljefe@206a84a. could you review it whether your changes are included completely. Otherwise I'll need to start with a fresh branch, which is fine by me.

@addaleax
Copy link
Member

@eljefedelrodeodeljefe It looks like they are but I don’t think git will accept it this way when you’re trying to land?

@eljefedelrodeodeljefe
Copy link
Contributor Author

I start over then.

@eljefedelrodeodeljefe
Copy link
Contributor Author

I am gonna close in favor for @addaleax' linked issue and discussion on Twitter. Further work doesn't make sense, due to the current nature of the implementation. Imo we should have a discussion about running this as a separate project with its own scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants