-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
I am psyched about this! Will probably not get to a review until tomorrow though. Thanks! |
@euclio This is great! |
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.
thanks a ton, overall this looks great! two inline questions for discussion
README.md
Outdated
conforms to a particular interface. | ||
|
||
- The frontend must allow a single positional argument. This argument will be | ||
the path to the documentation JSON generated by the backend. |
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.
yeah, I wonder if maybe streaming over stdin isn't better. I feel like it's a tradeoff; my frontend would need to take that stdin and write it to a file anyway, whereas your implementation currently needs to read the file outputted.
I think I'm going to argue that it should be over stdin, because in that case, it only needs to write a file if it actually needs to write a file, otherwise we've left an extraneous file around. Thoughts?
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.
Yeah, I think stdin makes more sense. Certainly if we want to support streaming output.
src/bin/rustdoc-ember.rs
Outdated
@@ -0,0 +1,85 @@ | |||
#[macro_use] |
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.
what do you think about making this a workspace, with rustdoc-ember
as a crate inside of it? This would let us get rid of the build script for the main rustdoc
crate, which feels cleaner to me.
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.
I'd prefer a workspace as well, but I don't think it's possible to force all crates be built? I'm trying to avoid the situation where you only build the backend and then the frontend can't be found. Though, this does already happen if you just cargo run --bin rustdoc
without cargo build
. With a workspace you'll have to remember to cargo build --all
instead of just cargo build
. If that's acceptable I'm happy to make it into a separate crate.
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.
It is, IMO.
Rebased and ready for review. |
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.
Awesome, thank you! So pumped about this.
This PR extracts the ember-specific logic into a
rustdoc-ember
binary. Alternative frontends may be used by setting theRUSTDOC_FRONTEND
environment variable.I've also updated the README. Does the proposed interface for frontends make sense? I also considered sending
data.json
through stdin.Fixes #52.