-
Notifications
You must be signed in to change notification settings - Fork 98
Log warning message when failing to generate graph #100
Conversation
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.
Works as advertised 😉
PS: Not tied to this PR but I think emphasizing that GraphViz is a peer dependency would prevent users from getting confused by this error message. Doesn't make for a good experience to see this error on first run. Either that or skipping this code branch when not running the diagram
subcommand.
Thanks for reviewing this, @kaishin!
I agree that this is an important detail to get right. Do you have a specific idea for where we can / should communicate To clarify: The GraphViz binary is only used to generate SVG diagrams in the HTML output of the |
Thanks for the clarification!
The first idea I got was to have a "Peer Dependencies" subsection in the readme. If that's too much then a note right below where it's first mentioned could do the job. A better way would be to let the user know whenever they run the |
I'm not as familiar with the term "peer dependency". Based on this write-up, I don't think this is quite the right term. But! It would be a great way to describe any generator plugin system we might add in the future. For now, I added GraphViz as an "(optional) requirement" (oxymoronic though that may be) with da2cd6c. That seemed like a reasonable first step.
I think warning on failure is a reasonable solution. It has the advantages of not making extra work and not having the potential for false positive or negative results. Like with validating email addresses, the only real way to see if an email is valid is to send a message and see if they get it. |
I agree that the term “peer dependency” isn’t quite right here. I also thought of “soft” and “indirect”. At any rate, what you proposed is probably enough for now. If it isn’t if won’t be the last time this comes up 😉 |
@kaishin Indeed. Thanks for sharing your thoughts about this. All really valuable groundwork as we continue to negotiate and evolve how all the different pieces fit together here. |
Resolves #48
As of #52, we log any errors caught during graph generation:
Although this involves error handling, the
error
log level is inappropriate here, because the failure isn't critical. Documentation may be generated even if GraphViz graphs aren't available.This PR changes the log level to
warning
and wraps the rawNSError
with some more actionable information:I was able to reproduce #52 locally by running
brew unlink graphviz
. Since the formula forswift-doc
now includes GraphViz as an (optional) dependency, this is much less likely to occur going forward.