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

Fix merge manifest error #267

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Fix merge manifest error #267

merged 1 commit into from
Feb 5, 2024

Conversation

JoonghyunCho
Copy link
Member

@JoonghyunCho JoonghyunCho commented Dec 1, 2023

Fix merge manifest error when main project doesn't have privileges and acount.

Updated code is following

                    else if (subapp.Name.LocalName == "privileges")
                    {
                        if (mainDoc.Root.Element(ns + "privileges") == null)
                        {
                            mainDoc.Root.Add(subapp);
                        }
                        else
                        {
                            mainDoc.Root.Element(ns + "privileges").Add(subapp.Descendants(ns + "privilege"));
                        }
                    }
                    else if (subapp.Name.LocalName == "account")
                    {
                        if (mainDoc.Root.Element(ns + "account") == null)
                        {
                            mainDoc.Root.Add(subapp);
                        }
                        else
                        {
                            mainDoc.Root.Element(ns + "account").Add(subapp.Descendants(ns + "account-provider"));
                        }
                    }

@sung-su
Copy link
Contributor

sung-su commented Dec 26, 2023

Thank you for the fixing !
But, it seems that there is a problem with CRLF - LF.
In a typical C# project, only the sln file is enforced to have CRLF.
(dotnet/templating#1248)
But it would be good to have it decided.
If you had to choose one, which one do you prefer, CRLF or LF?

@JoonghyunCho JoonghyunCho force-pushed the merge-manifest branch 2 times, most recently from 8a8e7b0 to 6e8d3eb Compare February 5, 2024 02:48
@JoonghyunCho
Copy link
Member Author

@sung-su Thanks, I updated the code format as it was.

Copy link
Contributor

@sung-su sung-su left a comment

Choose a reason for hiding this comment

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

LGTM

@sung-su sung-su merged commit c27e8af into main Feb 5, 2024
2 checks passed
@sung-su sung-su deleted the merge-manifest branch February 5, 2024 04:03
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