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

Obsolete the AnsiConsoleFactory class #585

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Oct 6, 2021

AnsiConsoleFactory should be an internal static class. Creating an IAnsiConsole can be achieved through Spectre.Console.AnsiConsole.Create(AnsiConsoleSettings)

Since AnsiConsoleFactory is public, it can't be changed from a non static class to a static class so obsoleting it is the second best thing to do.

Eventually, when AnsiConsoleFactory becomes internal and static, AnsiConsole.Create can be simplified to this (and the private static field _factory can be removed)

public static IAnsiConsole Create(AnsiConsoleSettings settings)
{
    return AnsiConsoleFactory.Create(settings);
}

@phil-scott-78
Copy link
Contributor

I'll need to wait for @patriksvensson to look because he'll know the historical reason for this being in it's current state.

But I just wanted to ask if in the future an issue could be created before the pull request. That way we can have a discussion before you go through the trouble of doing the PR, plus it allows me to review the requests with context from the other maintainers on how we want things to be resolved.

@0xced 0xced force-pushed the Obsolete-AnsiConsoleFactory branch from 81a5893 to 5ba13c8 Compare November 10, 2021 08:45
@0xced
Copy link
Contributor Author

0xced commented Nov 10, 2021

I will try to think about opening issues first for discussing.

For this particular case, it was not much trouble, though. If this change is not deemed appropriate, then just close the pull request without merging, I won't be offended. 😉

@phil-scott-78
Copy link
Contributor

This is another PR I think we are good with moving forward with no. But because it came before the big global namespace PR it does need to have conflicts resolved

@0xced
Copy link
Contributor Author

0xced commented Feb 28, 2022

Just as I predicted. 😜

Anyway, I can have a look next week but if you want to take over this PR earlier, please go ahead, it's trivial if I remember correctly.

`AnsiConsoleFactory` should be an internal static class. Creating an `IAnsiConsole` can be achieved through `Spectre.Console.AnsiConsole.Create(AnsiConsoleSettings)`

Since `AnsiConsoleFactory` is public, it can't be changed from a non static class to a static class so obsoleting it is the second best thing to do.

Eventually, when `AnsiConsoleFactory` becomes internal and static, `AnsiConsole.Create` can be simplified to this (and the private static field `_factory` can be removed)

```
public static IAnsiConsole Create(AnsiConsoleSettings settings)
{
    return AnsiConsoleFactory.Create(settings);
}
```
@nils-a nils-a force-pushed the Obsolete-AnsiConsoleFactory branch from 5ba13c8 to 391d5a0 Compare March 22, 2022 20:25
Copy link
Contributor

@nils-a nils-a left a comment

Choose a reason for hiding this comment

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

LGTM

@nils-a nils-a added this to the 0.44 milestone Mar 22, 2022
@nils-a nils-a merged commit 7998ece into spectreconsole:main Mar 22, 2022
@nils-a
Copy link
Contributor

nils-a commented Mar 22, 2022

@0xced your changes have been merged, thanks for your contribution 👍

@0xced 0xced deleted the Obsolete-AnsiConsoleFactory branch May 24, 2022 13:49
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.

3 participants