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

Change SvgSymbol namespace. #556

Merged
merged 5 commits into from Apr 24, 2021
Merged

Change SvgSymbol namespace. #556

merged 5 commits into from Apr 24, 2021

Conversation

H1Gdev
Copy link
Contributor

@H1Gdev H1Gdev commented Aug 15, 2019

Reference Issue

ref. #93.

What does this implement/fix? Explain your changes.

Only Svg.Document_Structure, _(underscore) is included in namespace.
So, change namespace without _(underscore).

I think Svg.Symbol is good.(like SvgDefinitionList, SvgFragment and SvgGroup.)
However, because there is a possibility of build error in existing environments, I decided on this namespace.

Any other comments?


namespace Svg.Document_Structure
namespace Svg.DocumentStructure
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I understand that this namespace fits better into the naming schema, but are you duplicating the whole class for this? I'm not sure that it is worth it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrbean-bremen

Svg.Document_Structure.SvgSymbol will be removed.
So I added Obsolete.

@mrbean-bremen
Copy link
Member

Also referring this to @tebjan - I'm unsure about the policy here.

@mrbean-bremen
Copy link
Member

I'm not happy with this duplicated code just for the sake of a more convenient namespace name. If there were a way to just add an alias to the namespace it would be ok. As far as I know, a namespace alias can only be used on import and cannot be exported, but hopefully someone knows a better possibility?

@H1Gdev H1Gdev force-pushed the symbol branch 2 times, most recently from 4c6ccfc to 9e2ca3c Compare January 15, 2020 06:17
@H1Gdev
Copy link
Contributor Author

H1Gdev commented May 15, 2020

@mrbean-bremen

SvgSymbol seems to be stable class, so merge this PR ?

@H1Gdev
Copy link
Contributor Author

H1Gdev commented Jan 24, 2021

rebase master.

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

I like this better, thanks! Can you add an entry in the release notes for the changed API?

@H1Gdev
Copy link
Contributor Author

H1Gdev commented Jan 28, 2021

@mrbean-bremen

I wrote the following in this PR:

I think Svg.Symbol is good.(like SvgDefinitionList, SvgFragment and SvgGroup.)
However, because there is a possibility of build error in existing environments, I decided on this namespace.

But, still do not need this namespace, so I decided to remove it in this commit.
This commit, will CS0104 build error in code, such as the following.

using Svg.Document_Structure;

...


var symbol = new SvgSymbol // error. but ok if Svg.Document_Structure.SvgSymbol
{
    Fill = new SvgColourServer(Color.Red),
    Stroke = new SvgColourServer(Color.Black),
    StrokeWidth = 2
};

However, I think this error would make users aware of the change.(It's easy to fix.)
(Of course, it will run correctly in pre-built executables.)

@mrbean-bremen
Copy link
Member

While this looks ok to me, I'm still a bit unsure about the consequences to users.
@wieslawsoltes, can you please have another look?

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

I'm going to merge this now, I think it is ok as is.

@mrbean-bremen mrbean-bremen merged commit 8c5c7c2 into svg-net:master Apr 24, 2021
github-actions bot pushed a commit that referenced this pull request Apr 24, 2021
…md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Change SvgSymbol namespace from Svg.Document_Structure to Svg BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Add compatibilty wrapper with old namespace
@H1Gdev H1Gdev deleted the symbol branch April 25, 2021 03:10
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.

None yet

2 participants