dmy / elm-doc-preview

Elm offline documentation previewer
https://www.npmjs.com/package/elm-doc-preview
Other
128 stars 15 forks source link

Report elm compiler errors for doc generation #41

Closed mdgriffith closed 3 years ago

mdgriffith commented 3 years ago

Hey Rémi!

Currently if elm-doc-preview is run on a project and the elm-compiler fails due to something like a missing doc comment, the error that is reported is just that something failed.

This PR captures the error from the elm-compiler, prints it out with nice coloring, and then exits.

Screen Shot 2020-12-09 at 8 21 29 PM

Thanks for this tool, it's wonderful.

rlefevre commented 3 years ago

Hi Matthew,

I guess it's useful mainly with --output option in CI scripts because compilation errors are already reported in the browser without this option. Right?

Also at first glance, I doubt we really want to exit when --output is not used because the error can be fixed, which will trigger the file watcher and the build and maybe fix the documentation in interactive sessions. Using elm-doc-preview interactively when editing documentations is an important focus of the application, and therefore so is fixing fatal errors without restarting elm-doc-preview.

Thank you for the PR!

mdgriffith commented 3 years ago

Yeah that's right, this is for when --output is used.

That makes total sense, I've removed the process.exit.

Does it make sense to print these errors in the terminal if --output is not specified? 🤔 I can thread through the output flag if we want them to be silent in the interactive-session case.

Our current usecase is that we're building an automatic styleguide generator for our app based on the docs, so we're using elm-doc-preview as a step in our tool to generate the docs.json from our application, though, we're using our own renderer from there.

(Also, probably should have talked to you first before creating a PR 😅 , but this was pretty quick to write, solved our internal challenge, and I figured it could start a discussion)

rlefevre commented 3 years ago

I'm not sure it's useful to print them in the terminal without --output. Do you see a use case?

I think either you work with the browser, either you work with --output, so duplicating the error does not seem really useful, but maybe someone has a use case for it:thinking:

mdgriffith commented 3 years ago

I can't think of a usecase either, I think you're right. I'll make it so they're only shown if output if specified.

Worst case scenario: if someone finds a usecase where they want them without --output, then it'll be an easy change.

rlefevre commented 3 years ago

Sounds good. The PR looks good too, so if you can just check that it works with Node.js >=8.10.0 (the minimum supported version), I'm ok to merge it and release a patch version. Else I will check myself later, no worries.

mdgriffith commented 3 years ago

Just ran it using v8.10.0 and everything worked as expected 🎉