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.

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

Refactor GenRTF() and GenHTML() to use shared components #3538

Closed
carlos-zamora opened this issue Nov 12, 2019 · 2 comments
Closed

Refactor GenRTF() and GenHTML() to use shared components #3538

carlos-zamora opened this issue Nov 12, 2019 · 2 comments
Assignees
@carlos-zamora
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone
Terminal v2.0

Comments

@carlos-zamora
Copy link
Member

Description of the new feature/enhancement

#3535 adds GenRTF() to enable RTF Copy. It has some parts that are very similar to GenHTML(), which makes sense. It'd be nice to find a way to refactor the code a bit to reuse some sections.

My concern is that we make find a bug and fix it in one and not the other.

Proposed technical implementation details (optional)

We could probably use a few helper methods to create a more distinct separation of HTML vs RTF specific portions. The overall architecture and flow of both functions are essentially the same. Maybe even add an enum?

Bonus points

Make GenHTML() noexcept too.
@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Nov 12, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 12, 2019
@carlos-zamora carlos-zamora added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 12, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal Backlog milestone Nov 14, 2019
@carlos-zamora carlos-zamora self-assigned this May 12, 2020
@carlos-zamora carlos-zamora mentioned this issue May 21, 2020
Open
25 tasks
@carlos-zamora
Copy link
Member Author

@DHowett I was taking a closer look at the differences between GenRTF() and GenHTML(). Refactoring them would just make the code more difficult to understand and risks breaking something. Mind if I just close this issue?

@DHowett
Copy link
Member

DHowett commented Jun 2, 2020

Go ahead!

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Terminal v2.0
Development

No branches or pull requests
3 participants
@DHowett @carlos-zamora @DHowett-MSFT