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

adds newline suppression for divs #366

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Conversation

wghilliard
Copy link
Contributor

@wghilliard wghilliard commented Jan 19, 2024

DISCLAIMER: I am a Microsoft employee making a contribution to this opensource project for use by downstream Microsoft products. If you would like to learn more, please contact cela@microsoft.com.

In some cases there is HTML that will wrap standard text in div tags like this

<div>hello</div><div>world</div>

The default behavior for the ReverseMarkdown library is to append two newlines for each item, a prefix and suffix \r\n per div tag, resulting in something that looks like this:


hello

world

In this PR, I propose adding a new Config option called SuppressNewlines that will remove the prefixed newline from div tags not enclosed in td tags, such that the output may look like the following:

hello
world

also

would it be okay if the dotnet framework 4.7.2 builds were re-enabled? my product doesn't currently support dotnet core :(

@mysticmind
Copy link
Owner

mysticmind commented Jan 19, 2024

@wghilliard Acknowledge you PR, few comments:

  • First to the easy bit, I removed support for dotnet framework 4.7.2 in the recent release thinking that no one would be using it given that dotnet core has become so much a main stream thing. I will add it back, no worries.
  • With regards to suppressing new lines, with the PR which you have sent, if the contiguous div elements don't have a parent then not adding the newline will result in altered Markdown behavior when you have other block tags above or below it. Since it is applied based on config, I am okay with pulling in the changes, it does not alter the default/correct behavior. I want you to think carefully on this and revert with your thoughts on why you want to do this. Alternative is to have a pre-processing step by parsing it using (say HtmlAgilityPack) and alter the pattern of div's as in your example to convert <div>hello</div><div>world</div> to hello<br>world.

On a lighter note, I have observed that many Microsoft folks use the library but never intend to support with say a GitHub sponsor to help the maintainer to maintain the library on an ongoing basis. I will be more than glad and appreciate If you could nudge your stakeholders at Microsoft to help with a sponsor.

@wghilliard
Copy link
Contributor Author

wghilliard commented Jan 19, 2024

I will add it back, no worries.

Thank you so much!

if the contiguous div elements don't have a parent then not adding the newline will result in altered Markdown behavior when you have other block tags above or below it.

I don't quite follow, do you have an example / test case you could share?

Alternative is to have a pre-processing step by parsing it using (say HtmlAgilityPack) and alter the pattern of div's as in your example to convert

hello
world
to hello
world.

This could be a viable solution, I'm not certain how the pattern matching works with HtmlAgilityPack. I can imagine defining the base case would be straight forward however it's not clear if an exhaustive list would be necessary in order to catch nested structures.

I will be more than glad and appreciate If you could nudge your stakeholders at Microsoft to help with a sponsor.

I have registered this library in the catalog of opensource dependencies, however I'm very removed from the decision makers. I can nominate it for the FOSS Fund, however the project will need to satisfy some additional criteria that is not currently met. We could discuss this more offline.

@mysticmind
Copy link
Owner

mysticmind commented Jan 20, 2024

@wghilliard quick note, I am down with a flu and will push out a release by Monday once I am feeling better. In retrospect, when I took a deeper look, what you have implemented is fine since the other block elements surrounding it will render their newlines appropriately. So ignore the concern which I raised.

I can nominate it for the FOSS Fund, however the project will need to satisfy some additional criteria that is not currently met. We could discuss this more offline.

I would like to learn more, you could DM me on twitter handle @mysticmindB, we could discuss it offline.

@wghilliard
Copy link
Contributor Author

I don't actually have an active twitter account, but I've sent you an email (as listed on your github profile) from my @msft email address!

@wghilliard
Copy link
Contributor Author

if we're okay with this implementation, let's ship it!

@mysticmind mysticmind merged commit 7fb45ec into mysticmind:master Jan 23, 2024
1 check passed
@mysticmind
Copy link
Owner

@wghilliard Check the latest release v4.3.0, it has support for your changes and also net46. There is one small change, the config name is SuppressDivNewlines.

@wghilliard
Copy link
Contributor Author

excellent, thank you!!

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.

2 participants