Skip to content

Conversation

amoerie
Copy link

@amoerie amoerie commented Dec 22, 2018

I'm recreating my original PR here, as requested in aspnet/Configuration#816 (comment) by @ajcvickers

I know this is a considerable change in the existing code, but all existing tests are still intact and green. The "Name" feature is still completely supported and as such, this is not a breaking change.

Addresses #485
Addresses #761

…figuration XML files.

- Maintain a stack of elements that are encountered during traversal of the XML file
- Detect sibling elements and automatically append indexes to the generated configuration keys, exactly like how the JSON configuration providers does for JSON arrays
- Add unit tests to verify no existing features were broken in the process
-  Existing tests were not modified, i.e. this is not a breaking change.
…own for duplicate keys

This is obviously no longer the case, repeated elements now contribute to the key.
@amoerie
Copy link
Author

amoerie commented Feb 27, 2019

@ajcvickers Is there anything holding back this PR? Other than busy schedules. 😉

@ajcvickers
Copy link

@amoerie Nope. Just time.

@amoerie
Copy link
Author

amoerie commented Feb 27, 2019

Fair enough, thanks!

@hacst
Copy link

hacst commented Mar 29, 2019

Looking forward to this landing. Not being able to express lists like I was used to from json configs when the app uses xml instead was quite unexpected.

Is there a rough timeframe for this landing by now?

@Jan-Magerl
Copy link

Jan-Magerl commented May 7, 2019

With this solution you are still not able to use IOptions

public class Config
{
  public List<Item> Items { get; set};
}
services.Configure<Config>("Section")

will not fill the list.

This is because of the ItemName don't exists in Jsonfiles.

{
  "Section":"{
    "Items": [
      {
         "Name": "item1"
      },
      {
         "Name": "item2"
      }
}

JsonConfigurationProvider would build paths like
Section:Items:0:Name
Section:Items:1:Name

XmlConfigurationprovider build the path

<configuration>
  <section>
    <Items>
      <Item>
        <Name>Item1</Item>
      </Item>
      <Item>
        <Name>Item2</Item>
      </Item>
    </Items
  </section>
</configuration>

Will generate
Section:Items:Item:0:Name
Section:Items:Item:1:Name

But IOptions don't want the Item part

Is there any possibility to remove "Item"?

@amoerie
Copy link
Author

amoerie commented May 7, 2019

@Jan-Magerl

If you don't want "Item" to appear in the config list, I believe you can do the following: (with the changes from this PR)

<configuration>
  <section>
    <Items><Name>Item1</Name></Items>
    <Items><Name>Item2</Name></Items>
  </section>
</configuration>

@Jan-Magerl
Copy link

@Jan-Magerl

If you don't want "Item" to appear in the config list, I believe you can do the following: (with the changes from this PR)

I also want to use XmlSerializer to write to the File, without weird Attributes.
I have found a Solution that works for me:

Change Line 224:

    public string GetKey()
    {
      var tokens = new List<string>(3);
      // root element does not contribute to prefix
      if (Parent != null && !Multiple) tokens.Add(ElementName);
      ...

This will fix my Issue, but maybe the behavior of IOptions should be fixed.

@jez9999
Copy link

jez9999 commented Feb 12, 2020

I really wish this would get merged. I could do with some array-like behaviour in my XML configuration.

@ghost
Copy link

ghost commented Mar 26, 2020

As per aspnet/Announcements#411, we are currently migrating components from this repository to other repositories. This PR targets components that have been moved to dotnet/runtime, in the src/libraries directory. If you're still interested in contributing this change, please retarget your PR to dotnet/runtime and reference the original issue discussing the change or bug fix. If you have questions, or need guidance on how to port your change, please tag @dotnet/extensions-migration in a comment and we'll try to help.

@amoerie
Copy link
Author

amoerie commented Mar 26, 2020

As per aspnet/Announcements#411, we are currently migrating components from this repository to other repositories. This PR targets components that have been moved to dotnet/runtime, in the src/libraries directory. If you're still interested in contributing this change, please retarget your PR to dotnet/runtime and reference the original issue discussing the change or bug fix. If you have questions, or need guidance on how to port your change, please tag @dotnet/extensions-migration in a comment and we'll try to help.

I've already migrated this pull request once. This pull request has been left open for more than a year. Obviously, there is no interest to actually merge this, so I don't think I'll spend the time again.

@jez9999
Copy link

jez9999 commented Mar 28, 2020

@amoerie That's a shame. There's an issue with Serilog config where it requires sinks to be in an array and a property in each array object is actually called...... "Name". So it clashes with the XML "name" attribute requirement. Could really do with a fix.

@DanielSundberg
Copy link

I would also like to see this merged. I stumbled into this issue while trying to add some legacy config files to a new dotnet core project.

@amoerie
Copy link
Author

amoerie commented Apr 7, 2020

@amoerie That's a shame. There's an issue with Serilog config where it requires sinks to be in an array and a property in each array object is actually called...... "Name". So it clashes with the XML "name" attribute requirement. Could really do with a fix.

It's not that I'm unwilling to do the work again, it's that I'm not convinced this PR would get any attention, exactly as before. If I can get an "official" confirmation that this PR would be strongly considered for merging, I am willing to make it happen again.

@maryamariyan
Copy link

maryamariyan commented Oct 20, 2020

It's not that I'm unwilling to do the work again, it's that I'm not convinced this PR would get any attention, exactly as before. If I can get an "official" confirmation that this PR would be strongly considered for merging, I am willing to make it happen again.

@amoerie I see that your PR was blocked twice because of two different repo consolidations. The source code is now moved to dotnet/runtime. Are you still interested in pushing a PR for this?

@amoerie
Copy link
Author

amoerie commented Oct 22, 2020

Hi @maryamariyan I made the pull request here: dotnet/runtime#43722

I'm glad to see this is being followed up on.

@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants