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

Traverse<T>() and TraverseDepthFirst<T>() show as extension methods on everything #889

Closed
QuantumToasted opened this issue Sep 1, 2021 · 2 comments · Fixed by #890
Closed

Comments

@QuantumToasted
Copy link

QuantumToasted commented Sep 1, 2021

I use the library as a small part of a project I work on, for processing SVG images downloaded from the internet, and it has been bothering me for awhile that these two methods show up as extensions methods for everything, including things that have absolutely nothing to do with this library.

Would it be possible to add a constraint on these methods or otherwise make them internal? Having a no-constraint generic extension method on a public API for a project dedicated to a specific line of work feels like a potential problem and at the very least, an eyesore for consumers.

This would be a breaking change to do either, and I have no clue how many people use these extensions outside of their internal use by the library, but it would be greatly appreciated.

I know this is totally splitting hairs for some, but for others like me it's a genuine annoyance when trying to work with other objects and seeing these extension methods on everything.

If absolutely desired, one could simply slap [EditorBrowsable(EditorBrowsableState.Never)] on them to not introduce a breaking change, but some IDEs like Rider still ignore this for some reason.

@QuantumToasted QuantumToasted changed the title Traverse<T>() and TraverseDepth<T>() show as extension methods on everything Traverse<T>() and TraverseDepthFirst<T>() show as extension methods on everything Sep 1, 2021
@mrbean-bremen
Copy link
Member

As far as I can see, these methods have been added to optimize ApplyRecursive and ApplyRecursiveDepthFirst. I'm not sure if it was intended to expose them, but for me it looks like making them internal would be ok. I doubt that they have been used externally.
Any other opinions here?
CC @H1Gdev , @tebjan

@tebjan
Copy link
Contributor

tebjan commented Sep 1, 2021

Yes, agreed. It's mainly for internal use.

mrbean-bremen added a commit to mrbean-bremen/SVG that referenced this issue Sep 1, 2021
- avoids polluting the external API
- fixes svg-net#889
mrbean-bremen added a commit to mrbean-bremen/SVG that referenced this issue Sep 2, 2021
- avoids polluting the external API
- fixes svg-net#889
mrbean-bremen added a commit that referenced this issue Sep 2, 2021
- avoids polluting the external API
- fixes #889
github-actions bot pushed a commit that referenced this issue Sep 2, 2021
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 a pull request may close this issue.

3 participants