Skip to content
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

Implement text renderer. #208

Closed
wants to merge 19 commits into from
Closed

Conversation

csadorf
Copy link

@csadorf csadorf commented Dec 12, 2019

For rendering of markdown documents in text format.

The concrete use case is the ability to render templated markdown documents directly within a terminal environment.

@csadorf
Copy link
Author

csadorf commented Dec 12, 2019

This patch enables a very concrete use case for signac and I would be interested in some feedback on whether there is any interest in merging this particular functionality into the package before I finalize this.

return f'`{text}`'

def strong(self, text):
return '**' + text + '**'
Copy link

Choose a reason for hiding this comment

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

Since signac intends to use this in a terminal context, it might be a nice touch to use terminal bolding. I've used this in other applications with several terminals / multiple platforms, so I think it works reliably.

Suggested change
return '**' + text + '**'
return '\033[1m' + text + '\033[0m'

Copy link

Choose a reason for hiding this comment

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

And maybe TerminalRenderer is a better name than TextRenderer? Not sure.

Copy link
Author

Choose a reason for hiding this comment

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

I recognize that I didn't fully commit to either text or terminal rendering both in the code and the branch name. 😄

I'm happy to fix that before moving forward.

@lepture
Copy link
Owner

lepture commented Jan 6, 2020

I'm not quite sure if it is ok to include this into mistune; how about making your own renderer a python project, because it is using bs4.

In mistune, I'd like to keep it dependency free.

@bdice
Copy link

bdice commented Jan 6, 2020

@lepture @csadorf I see a couple paths forward, feel free to chime in if you have other ideas:

  • The dependency on bs4 could be removed, with some work. If that was done, would this feature be suitable for inclusion in mistune?
  • If not, and we need to create our own renderer, we could still depend on mistune for the parsing and just implement our own rendering class (essentially copy-paste this PR into our project).

@csadorf
Copy link
Author

csadorf commented Jan 6, 2020

I'm not quite sure if it is ok to include this into mistune; how about making your own renderer a python project, because it is using bs4.

In mistune, I'd like to keep it dependency free.

@lepture The dependency on bs4 is a stop-gap solution to implement the prototype. The patch is currently a rough prototype which I would clean up before requesting the actual merge.

Despite the dependency, anything else that we we would need to change before you could accept the contribution?

@lepture
Copy link
Owner

lepture commented Jan 7, 2020

Is there a standard for this text renderer. I'm not quite sure what is the format you are using. So if it is for terminal, I see text **strong**, but there is actually a bold style with ansi escape. If you are meaning ansi escape for terminal, I'd like to call this renderer AnsiRenderer.

It is ok to include it in mistune if it is dependency free since terminal is a common use case.

@zhou-pj
Copy link

zhou-pj commented Jan 23, 2020

Is there a standard for this text renderer. I'm not quite sure what is the format you are using. So if it is for terminal, I see text **strong**, but there is actually a bold style with ansi escape. If you are meaning ansi escape for terminal, I'd like to call this renderer AnsiRenderer.

It is ok to include it in mistune if it is dependency free since terminal is a common use case.

@lepture I think we can write our own function to replace bs4 used in the prototype. The tabulate package is also used here, for this one, would it be ok to just copy the tabulate.py into this package?

@zhou-pj
Copy link

zhou-pj commented Feb 11, 2020

@vyasr this PR should be ready to go.

@vyasr
Copy link

vyasr commented Feb 11, 2020

@zhou-pj can you mark this as ready for review?

@zhou-pj
Copy link

zhou-pj commented Feb 11, 2020

@vyasr I don't think we have access to do that.
@lepture We think this PR is ready for your review.

@waylan
Copy link

waylan commented Jun 19, 2020

+1 for the AnsiRenderer. However, what is the point of the text renderer? I could see a markdown renderer which output cleaned up Markdown. However, this text renderer is incomplete. I see no support for nested list items (which should be indented). And it only supports unordered lists. Ordered lists raise an assertion error. And various methods fail to account for possible None values. Those were the immediately obvious ones from a quite glance at the code.

As an aside, I discovered this after I started work on my own markdown renderer. I am finding the renderer API very limiting for doing a proper full markdown renderer (properly numbering and incrementing ordered lists being a prime example). I would suggest that 2.0 final shouldn't be released until the renderer API supports doing a complete markdown renderer.

@csadorf
Copy link
Author

csadorf commented Jun 23, 2020

+1 for the AnsiRenderer. However, what is the point of the text renderer? I could see a markdown renderer which output cleaned up Markdown. However, this text renderer is incomplete. I see no support for nested list items (which should be indented). And it only supports unordered lists. Ordered lists raise an assertion error. And various methods fail to account for possible None values. Those were the immediately obvious ones from a quite glance at the code.

@waylan The use case is very specifically to have a canonical formatting that specifically handles proper formatting of tables, etc. The only difference between the ansi and the text renderer is that the former uses control characters for bold formatting etc., and the latter does not. I think it would be fine to remove the text rendered if this poses a roadblock, however it is probably fair to say that neither the pure text and the ansi renderer follow a specific standard, they just produce a formatted version of the marked up input.

With respect to the limitations, you are absolutely correct, that this implementation is incomplete. I created this draft PR with the help of @zhou-pj and @vyasr to get some feedback on the approach and to gauge whether there is general interest to merge this into the code base. I unfortunately did not have any time to prioritize this lately, which is why in the end we decided to vendor a forked version of the package that works for our purposes. However, I am happy to provide assistance in moving this forward.

@csadorf
Copy link
Author

csadorf commented Oct 5, 2021

It appears there is no interest in this.

@csadorf csadorf closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants