Remove parsers by Rosuavio · Pull Request #3759 · CommunityToolkit/WindowsCommunityToolkit

Rosuavio · 2021-02-16T22:53:25Z

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)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
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 · 2021-02-16T22:53:29Z

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 🙌

Rosuavio · 2021-02-16T23:10:37Z

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.

ghost · 2021-02-17T16:46:08Z

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 · 2021-02-17T17:40:21Z

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

michael-hawker

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

michael-hawker · 2021-02-17T20:06:22Z

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


@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?

michael-hawker · 2021-02-17T20:06:46Z

Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj

    <Compile Include="SamplePages\ListViewExtensions\ListViewExtensionsPage.xaml.cs">
      <DependentUpon>ListViewExtensionsPage.xaml</DependentUpon>
    </Compile>
-    <Compile Include="SamplePages\MarkdownParser\MarkdownParserPage.xaml.cs">


Nice catch! 👍

michael-hawker · 2021-02-17T20:08:35Z

Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Render/MarkdownRenderer.cs

    /// Generates Framework Elements for the UWP Markdown Textblock.
    /// </summary>
-    public partial class MarkdownRenderer : MarkdownRendererBase
+    internal partial class MarkdownRenderer : MarkdownRendererBase


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?

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

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.

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

ghost · 2021-02-17T21:38:39Z

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

Move Parsers code into markdown package.

Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Parsers/Constants.cs

Rosuavio · 2021-02-24T20:40:04Z

Completes #3654

ghost requested review from Kyaa-dost, azchohfi and michael-hawker February 16, 2021 22:53

ghost added the markdown 📑 Issues related to Markdown label Feb 16, 2021

Rosuavio marked this pull request as ready for review February 16, 2021 23:06

Kyaa-dost approved these changes Feb 17, 2021

View reviewed changes

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

michael-hawker suggested changes Feb 17, 2021

View reviewed changes

ghost added needs author feedback 📝 needs attention 👋 and removed needs author feedback 📝 labels Feb 17, 2021

Rosuavio added 6 commits February 18, 2021 10:25

Remove Parsers pacakge.

c48d2bc

Move Parsers code into markdown package.

Remove MarkdownParserPage

f91aaef

Remove Parsers smoke test

2c25add

Mark all Parsing code as depricated

a79c96b

Dissable warnings from using dpericated Parsing code

675efdd

Add missing header

6b1f0a2

michael-hawker reviewed Feb 18, 2021

View reviewed changes

Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Parsers/Constants.cs Outdated Show resolved Hide resolved

michael-hawker approved these changes Feb 18, 2021

View reviewed changes

michael-hawker added 2 commits February 18, 2021 13:57

Update Microsoft.Toolkit.Uwp.UI.Controls.Markdown/Parsers/Constants.cs

6cb4eb5

Merge branch 'master' into remove-parsers

aa12b00

ghost merged commit defce5d into CommunityToolkit:master Feb 18, 2021

Rosuavio deleted the remove-parsers branch February 19, 2021 16:16

This pull request was closed.

Conversation

Rosuavio commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #3744

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

Uh oh!

ghost commented Feb 16, 2021

Uh oh!

Rosuavio commented Feb 16, 2021

Uh oh!

ghost commented Feb 17, 2021

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.

Uh oh!

Rosuavio commented Feb 17, 2021

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

michael-hawker Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Rosuavio Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Rosuavio Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 17, 2021

Uh oh!

Uh oh!

Rosuavio commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rosuavio commented Feb 16, 2021 •

edited

Loading

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.