@grimmi Awesome, thanks for doing this! I added “Fixes #2298” to your PR body because that tracks and closes that issue.

This looks good to me, but I agree with @jnm2 that we should use the same names in the XML as IDictionary exposes. Please rename entry to item and name to value. Once that is done, I think we can merge. I am going to give my approval on the assumption that this change will be made.

@grimmi I am hoping to get the 3.8 release out in the next day or two. Do you think you could knock this off by then? I am going to add the associated issue to the milestone so I can track it.

Yep, going to do it this evening. I'm out at the moment, but will do it some time tonight.

@rprouse Just checking, looks like you meant changing name to key, and a veto on moving the value attribute contents to the element contents?

I'm dubious about the use of <item> because it seems much too general. As it is, any code looking at a <setting> will now have to distinguish between settings with a value attribute and settings with inner XML as it's value. If, in the future, we add other collection types, we will have to distinguish <item> entries that contain key and value attributes from those that only contain values. This is pretty complicated to do, particularly in a transform. Of course, our code can handle it, but I'm thinking about people who post-process the XML.

There are two ways to deal with possible future change. One is to wait for the future, which we all know is my preferred approach. Change is inevitable but any particular change is not. Preparing for the future often means preparing for the wrong future.

The other approach is to act in a way that leaves everything open.

In fact, the first approach is actually a degenerate case of this one - doing nothing leaves everything open. But I'm talking about doing something here. 😄

The approach being picked seems to fall somewhere between those two choices. If we really want to be general, then the suggestion of using inner text or xml as the value is the most general. Simply picking a particular attribute or element name only works if some particular future arrives.

As I stated on the issue, I'm backing off this one. Frankly, it looks to me as if it is being decided in haste in order to get it into the 3.8 release. @ChrisMaddock originally designated this as an easyfix, probably thinking we should do the minimum in order to solve the immediate issue. But if folks want to generalize for possible (?) future expansion, I think that the "easiness" drops out, even if the code is easy to write.

Point of procedure: When @rprouse approves, is that intended to cancel the "Changes Requested" by @jnm2? GitHub, of course, thinks not but that's how I perceive it. @rprouse ?

Regarding procedure, I agree with @CharliePoole. @rprouse should have the final say (but of cause we should try to get consensus about a given issue and its solution).
Ps. Sorry for the short comments, but I on my phone.

Rob can also feel free to dismiss my reviews for expediency. In this case my review was no longer relevant anyway. =)

@rprouse I hope this is what we ended up with in the discussion. You wrote to change name to value, but that was a typo, right?

Point of procedure: When @rprouse approves, is that intended to cancel the "Changes Requested" by @jnm2? GitHub, of course, thinks not but that's how I perceive it. @rprouse ?

In most cases, I don't think that my approval overrides others. It is just that if someone else on the team has requested changes, I feel that if I also request the same changes it slows the process down because now two team members need to come back and re-review. So my approval is pending the other team members requests being satisfied.

@grimmi there seems to be a fair amount of disagreement on this issue, sorry. Thanks for pushing the changes, I will re-review, but I am also going to go back and re-read the issue. With this much disagreement I am clearly missing something. 😄

I will get back to you soon...