Skip to content
This repository was archived by the owner on May 20, 2020. It is now read-only.

Support alternative frontends #191

Merged
merged 5 commits into from
Oct 17, 2017
Merged

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Oct 15, 2017

This PR extracts the ember-specific logic into a rustdoc-ember binary. Alternative frontends may be used by setting the RUSTDOC_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.

@steveklabnik
Copy link
Owner

I am psyched about this! Will probably not get to a review until tomorrow though. Thanks!

@mgattozzi
Copy link
Contributor

@euclio This is great!

Copy link
Owner

@steveklabnik steveklabnik left a 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.
Copy link
Owner

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?

Copy link
Contributor Author

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.

@@ -0,0 +1,85 @@
#[macro_use]
Copy link
Owner

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.

Copy link
Contributor Author

@euclio euclio Oct 17, 2017

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.

Copy link
Owner

Choose a reason for hiding this comment

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

It is, IMO.

@euclio
Copy link
Contributor Author

euclio commented Oct 17, 2017

Rebased and ready for review.

Copy link
Owner

@steveklabnik steveklabnik left a 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.

@steveklabnik steveklabnik merged commit 078db5b into steveklabnik:master Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants