-
Notifications
You must be signed in to change notification settings - Fork 66
Make rendering color agnostic #387
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
base: master
Are you sure you want to change the base?
Conversation
Very cool! I like the HTML backend, can see myself using that. Had a quick look, will have a closer look later in week. Before it is merged I think it needs some fixes after running |
im trying to run the tests and im getting an error in anyhow, error[E0599]: no method named `backtrace` found for reference `&(dyn std::error::Error + Send + Sync + 'st
atic)` in the current scope
--> /home/urisig/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/anyhow-1.0.0/src/error.rs:159:44
|
159 | .or_else(|| self.inner.error().backtrace())
| ^^^^^^^^^ method not found in `&dyn Error + Send + Sync``` |
Oooh I have run into that issue before... I don't think it was on this repository and I cannot remember how I fixed it. I can only think: double check you are on the latest and run |
tests are now passing. should this be locked behind the termcolor feature just like before? I think i might be able to get this to work without it. |
@kaleidawave the pr is ready to review. Although im not quite sure about the changes to the api yet, the emit function requires a bit more boilerplate to use now. Im also thinking of making the html renderer part of the crate, its a very simple renderer that can be quite useful |
@@ -104,51 +108,51 @@ pub enum DisplayStyle { | |||
pub struct Styles { |
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.
Is there a reason why these fields are no longer public?
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 used it to refactor and forgot to make it public, they definitely should be public
|
||
#[cfg(feature = "termcolor")] | ||
#[cfg(feature = "std")] | ||
impl<'a, W: WriteColor> io::Write for StylesWriter<'a, W> { |
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.
Nice! So this impl
ensures all existing uses will not break and this update will be backwards compatible?
codespan/src/file.rs
Outdated
.unwrap(); | ||
let style = codespan_reporting::term::Styles::default(); | ||
let color_writer = StandardStream::stdout(ColorChoice::Auto); | ||
let mut writer = codespan_reporting::term::StylesWriter::new(color_writer.lock(), &style); |
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.
Is there a reason why the writer has changed. StandardStream::stdout(ColorChoice::Auto)
should still work right? Want to make sure that it doesn't create breaking changes
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 provided a blanket impl for ColorWriter that should fix prevent this from being a very big change, but if you use custom styles you will still need to provide the styles with StylesWriter instead of passing it in the config. This definitely needs to be a version bump since it removes the styles field from Config.
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, when I have the time I will investigate whether some of the dependents of this project require any changes.
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, when I have the time I will investigate whether some of the dependents of this project require any changes.
Great, we can also add the html/svg renderer as part of the package or at least include it in the readme.
Looks good, have a few queries but after they are resolved and I have tested this on one of my own projects I will merge. |
@kaleidawave any progress? |
Hi @urisinger , apologies have been busy recently. As I have only recently become a maintainer and still don't have a lot of knowledge of the codebase and this change is breaking I have put this on the back-burner. I have found a few changes that would be needed in external repositories, I will add some more as I test others
But other repositories: I can merge this soon, but need 3 things:
Thanks! |
this commit fixed #389, it provided a blaket implementation of StylesWriter for ColorWriter that uses the default Color config. It might be possible to implement StylesWriter for a regular Writer too if the term feature is off. |
Resolves #386
This adds a new
WriteStyle
trait for styling that replaces WriteColor.Right now, the only public APIs I’ve changed are the ones for
Config
andemit
: I removedStyle
fromConfig
(because a renderer doesn’t always need one) and added astyle
parameter toemit
.Using the API is very simple, just implement the
WriteStyle
trait, and then pass your writer to the renderer. I’ve implemented a minimal HTML backend here, based on readme preview.