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

Issue2394 multiple markdown exporters not possible even with different names #2395

Conversation

bstordrup
Copy link
Contributor

Changed merging of exporters to check for duplicates by exporter.Name instead of the System.Type.

Added a test to verify that two MarkdownExporters with different named (GitHub and Atlassian) are accepted and both present in the final list (count is 2).

The original that removes duplicates still passes.

Fixes #2394

@timcassell
Copy link
Collaborator

timcassell commented Aug 9, 2023

Maybe it would be better to sub-class the MarkdownExporter rather than changing the logic. But I don't feel strongly about it either way.

@bstordrup
Copy link
Contributor Author

bstordrup commented Aug 10, 2023

Maybe it would be better to sub-class the MarkdownExporter rather than changing the logic. But I don't feel strongly about it either way.

That could be an option too, but I think it would complicate the class hierarchy. The different variations of MarkdownExporter are only differing in various values of the MarkdownExporter properties. So it would lead to specific classes not changing functionality but just setting properties. With that in mind, I would prefer the current way with declaring them as static properties on MarkdownExporter.

Also, there is no public constructor on MarkdownExporter class, which also complicates creating specific classes and instantiating these.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for providing the tests @bstordrup !

@adamsitnik adamsitnik merged commit d058c7b into dotnet:master Aug 10, 2023
7 checks passed
@bstordrup bstordrup deleted the Issue2394_MultipleMarkdownExportersNotPossible_EvenWithDifferentNames branch August 10, 2023 09:42
@timcassell timcassell added this to the v0.13.8 milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple MarkdownExporters not possible
3 participants