Skip to content

fix prefix dupplication error#155

Open
whistyun wants to merge 7 commits into
cwensley:developfrom
whistyun:develop
Open

fix prefix dupplication error#155
whistyun wants to merge 7 commits into
cwensley:developfrom
whistyun:develop

Conversation

@whistyun

Copy link
Copy Markdown

If there are some namespaces of the same acronym, XamlServices.Save cause XmlException with the message "'xmlns:p' is a duplicate attribute name".

Demonstration code

namespace Test
{
    class Program
    {
        static void Main(string[] args)
        {
            var list = new List<object>();
            list.Add(new Charlie());
            list.Add(new Carol()); // if this line comment out, no error occured

            var txt = Portable.Xaml.XamlServices.Save(list);
            Console.WriteLine(txt);
        }
    }
}

namespace Alpha.Bravo
{
    public class Charlie
    {
        public Chocolate FavoriteFood { set; get; } = new Chocolate();
    }
}

namespace Alice.Bob
{
    public class Carol
    {
        public Chocolate FavoriteFood { set; get; } = new Chocolate();
    }
}

namespace Apple.Banana
{
    public class Chocolate
    {
    }
}

This pull request avoids the above problem by adding an ordinal number at the end

When using XmlService.Save, the prefix "p" may be duplicated.
@whistyun

Copy link
Copy Markdown
Author

I think this build failure is bacause of it.
Should I modify yml file for workaround?

@cwensley

Copy link
Copy Markdown
Owner

Thanks for submitting the PR! This is great.

There's a few things.. would you be able to write it without having a capturing lambda or enumerable (foreach)? Also, a test would be great to have to ensure this doesn't break in the future.

@cwensley

Copy link
Copy Markdown
Owner

.. as for the build error, yes if you know what is needed to be done to fix It please update. I'll probably update this to use .NET 5 soon anyway.

@whistyun

whistyun commented Dec 22, 2020

Copy link
Copy Markdown
Author

would you be able to write it without having a capturing lambda or enumerable (foreach)?

I can't come up with a way to write without 'Any' method nor enumerable.
I'm going to avoid call 'Any' in while loop. However don't you mean it?

public string AddNamespace (string ns)
{
	var l = Namespaces;
	string prefix, s;

	...

	else
	{
		s = sctx.GetPreferredPrefix(ns);
		if (!l.Any(i => i.Prefix == s))
			prefix = s;
		else 
		{
			int getSuffix(string p)
				=> !p.StartsWith("p") ? 0 :
				   int.TryParse(p.Substring(1), out var i) ? i :
				   1;

			var idx = l.Max(i => getSuffix(i.Prefix)) + 1;
			prefix = s + idx;
		}
	}

	...
}

@cwensley

Copy link
Copy Markdown
Owner

How about creating a function:

		bool AnyHavePrefix(List<NamespaceDeclaration> namespaces, string prefix)
		{
			for (int i = 0; i < namespaces.Count; i++)
			{
				var ns = namespaces[i];
				if (ns.Prefix == prefix)
					return true;
			}
			return false;
		}

After looking into that method, I realize there's another use of an Any() with capturing lambda in that method too... There's no reason we couldn't make it more optimized while we notice it, I hope you don't mind.

Also, just a quick question.. why do you start at 2? Does System.Xaml behave this way?

@whistyun

whistyun commented Dec 22, 2020

Copy link
Copy Markdown
Author

When I checked System.Xaml, it started from 1.
I will match it with System.Xaml.

By the way, System.Xaml uses the acronyms, not the ‘p’ (for example, the demo code uses ‘ab’, ‘ab1’, ‘ab2’).

If possible, if GetPreferredPrefix returns 'p', I will use the acronyms instead.
If we can get the acronyms, I will use the acronyms and when prefix duplication is occurred, add suffix to the acronyms. If we can't get the acronym, use GetPreferredPrefix as an alternative.

@whistyun

Copy link
Copy Markdown
Author

@cwensley
I fixed to start from 1. and If a acronyms is gotten, it 's used.
I Also added some test cases.

Additionary, I added workaround build.yml to avoid build failure.

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.

2 participants