Skip to content

Remove parsers #3759

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.

Sign up for GitHub

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

Jump to bottom
Merged
8 commits merged into from
Feb 18, 2021
Merged

Remove parsers #3759

8 commits merged into from
Feb 18, 2021

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented Feb 16, 2021 •
edited
Loading

Fixes #3744

Move Parsing code from Microsoft.Toolkit.Parsers into Microsoft.Toolkit.Uwp.UI.Controls.Markdown. Make parsing code internal as we don't want anyone using our parsers.

Remove sample MarkdownParserPage as it directly uses our parser.

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)
  • Sample app changes

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Feb 16, 2021

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost February 16, 2021 22:53
@ghost ghost added the markdown 📑 Issues related to Markdown label Feb 16, 2021
@Rosuavio Rosuavio marked this pull request as ready for review February 16, 2021 23:06
@Rosuavio
Copy link
Contributor Author

So one thing to make sure works is the rendering of the docs for each sample, as this touches that functionality of the sample app.
Also some APIs that were already in Controls.Markdown were made internal. They exposed underlying access to the parsers and it seemed easiest to lock them down.

@Rosuavio Rosuavio added auto merge ⚡ for-review 📖 To evaluate and validate the Issues or PR next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. labels Feb 17, 2021
@ghost
Copy link

ghost commented Feb 17, 2021

Hello @RosarioPulella!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@Rosuavio
Copy link
Contributor Author

For anyone reviewing, this one is clearest commit by commit.

michael-hawker
michael-hawker requested changes Feb 17, 2021
View reviewed changes
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Question on structure here around the Renderer part and its relation to the underlying Parser.

Microsoft.Toolkit.Uwp.SampleApp/Controls/SampleAppMarkdownRenderer.cs
/// <summary>
/// A Rendering Superclass for the Markdown Renderer, allowing custom styling of Elements in Markdown.
/// </summary>
internal class SampleAppMarkdownRenderer : MarkdownRenderer
Copy link
Member

Choose a reason for hiding this comment

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

@RosarioPulella this is what helps display the docs within the Sample App, so I don't think we can remove it? Is it because the MarkdownRenderer would be internalized now? That could be a problem... I think we'd still want that class exposed?

Or did this just move somewhere else?

Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj
@@ -745,9 +744,6 @@
<Compile Include="SamplePages\ListViewExtensions\ListViewExtensionsPage.xaml.cs">
<DependentUpon>ListViewExtensionsPage.xaml</DependentUpon>
</Compile>
<Compile Include="SamplePages\MarkdownParser\MarkdownParserPage.xaml.cs">
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch! 👍

Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Render/MarkdownRenderer.cs Outdated
@@ -18,7 +18,7 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Render
/// <summary>
/// Generates Framework Elements for the UWP Markdown Textblock.
/// </summary>
public partial class MarkdownRenderer : MarkdownRendererBase
internal partial class MarkdownRenderer : MarkdownRendererBase
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should still expose the Renderer as part of the Control package as it allows customization of the control.

We just don't want the base parse itself to be exposed. Or are these two things more tightly coupled than we thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was having trouble keeping it public without having to make every parser class public. I have something I can try.

Copy link
Member

Choose a reason for hiding this comment

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

The other option we have is to mark the parser bit Obsolete if we need to leave things public? At least by moving it out of it's own package it's a stronger signal until we can refactor the MarkdownTextBlock in the future with #3200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went a head and marked most of the Parsing code as Obsolete.

@ghost
Copy link

ghost commented Feb 17, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@ghost ghost merged commit defce5d into CommunityToolkit:master Feb 18, 2021
@Rosuavio Rosuavio deleted the remove-parsers branch February 19, 2021 16:16
@Rosuavio
Copy link
Contributor Author

Completes #3654

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker approved these changes

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees
No one assigned
Labels
auto merge ⚡ for-review 📖 To evaluate and validate the Issues or PR markdown 📑 Issues related to Markdown needs attention 👋 next preview ✈️ Label for marking what we want to include in the next preview release for developers to try.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cleanup] Move Parsers to be internal to Markdown package, deprecate old package on NuGet
3 participants
@Rosuavio @michael-hawker @Kyaa-dost