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

Write TestParametersDictionary to xml result file in readable format #2298

Closed
ChrisMaddock opened this Issue Jul 4, 2017 · 39 comments

Comments

Projects
None yet
7 participants
@ChrisMaddock
Member

ChrisMaddock commented Jul 4, 2017

I just went to look up TestParameters in the results xml.

<setting name="TestParameters" value="1=d;2=e" />
<setting name="TestParametersDictionary" value="System.Collections.Generic.Dictionary`2[System.String,System.String]" />

It would be good if we can write that out TestParametersDictionary in a more readable fashion in the TestResult xml - the current output isn't much use.

@ChrisMaddock ChrisMaddock added the confirm label Jul 4, 2017

@oznetmaster

This comment has been minimized.

Show comment
Hide comment
@oznetmaster

oznetmaster Jul 4, 2017

Contributor

I was not aware that it was deprecated, informally or formally :(

Contributor

oznetmaster commented Jul 4, 2017

I was not aware that it was deprecated, informally or formally :(

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Jul 4, 2017

Member

Did I get it the wrong way around? I thought you were one of the key people pushing for it @oznetmaster! 😄

I lost track of all the debate that went into that one, but I thought one of TestParameters/TestParametersDictionary was intended to replace the other - but both were going to continue to be supported, for runner backwards compatibility? Am I wrong there?

Member

ChrisMaddock commented Jul 4, 2017

Did I get it the wrong way around? I thought you were one of the key people pushing for it @oznetmaster! 😄

I lost track of all the debate that went into that one, but I thought one of TestParameters/TestParametersDictionary was intended to replace the other - but both were going to continue to be supported, for runner backwards compatibility? Am I wrong there?

@oznetmaster

This comment has been minimized.

Show comment
Hide comment
@oznetmaster

oznetmaster Jul 4, 2017

Contributor

Not at all. I was actually opposed to the addition of TestParameterDictionary.

Contributor

oznetmaster commented Jul 4, 2017

Not at all. I was actually opposed to the addition of TestParameterDictionary.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Jul 4, 2017

Member

I removed my comment on the matter, given I clearly don't know what I'm on about. 🙂

Member

ChrisMaddock commented Jul 4, 2017

I removed my comment on the matter, given I clearly don't know what I'm on about. 🙂

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jul 4, 2017

Member

@ChrisMaddock I'm confused as well, so I guess we are forced to focus on the issue itself. 😄

Old frameworks only understand TestParameters. New frameworks currently understand both. Ideally, both shouldn't be in the XML IMO. They are two representations of the same thing.

Where is the code that adds the settings to the XML? I'm not finding it on my phone.

Member

CharliePoole commented Jul 4, 2017

@ChrisMaddock I'm confused as well, so I guess we are forced to focus on the issue itself. 😄

Old frameworks only understand TestParameters. New frameworks currently understand both. Ideally, both shouldn't be in the XML IMO. They are two representations of the same thing.

Where is the code that adds the settings to the XML? I'm not finding it on my phone.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Jul 4, 2017

Member

private static void AddSetting(TNode settingsNode, string name, object value)

Looks like it just ToString()'s it. Maybe we can do something more intelligent if the 'object' of the setting is a dictionary.

Member

ChrisMaddock commented Jul 4, 2017

private static void AddSetting(TNode settingsNode, string name, object value)

Looks like it just ToString()'s it. Maybe we can do something more intelligent if the 'object' of the setting is a dictionary.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jul 4, 2017

Member

My thought is that the framework should reflect the settings it's actually using. For new frameworks, that's the dictionary. So the legacy setting can easily be ignored. The TestParametersDictionary setting can be handled element by element, giving us something the actual values.

Member

CharliePoole commented Jul 4, 2017

My thought is that the framework should reflect the settings it's actually using. For new frameworks, that's the dictionary. So the legacy setting can easily be ignored. The TestParametersDictionary setting can be handled element by element, giving us something the actual values.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 17, 2017

Member

@CharliePoole - I'd like to mark this issue easyfix/up-for-grabs.

Your comment on the framework reflecting the settings it's using - I don't believe we currently make any changes to the FrameworkPackageSettings before they're written out. The same concept applies in other ways - e.g. I could pass in a RandomSeed and have no random tests, or I could pass in a string setting from a more recent runner to an older framework that doesn't know what to do with it - this would still be written out to the xml. Accordingly - I think, if we wanted to filter the settings, that would be a bigger and separate issue.

Are you happy for me to update this issue to just correct the issue at hand, writing something readable to the xml for TestParametersDictionary? And anything relating the filtering which settings are written out to be a separate issue?

Member

ChrisMaddock commented Aug 17, 2017

@CharliePoole - I'd like to mark this issue easyfix/up-for-grabs.

Your comment on the framework reflecting the settings it's using - I don't believe we currently make any changes to the FrameworkPackageSettings before they're written out. The same concept applies in other ways - e.g. I could pass in a RandomSeed and have no random tests, or I could pass in a string setting from a more recent runner to an older framework that doesn't know what to do with it - this would still be written out to the xml. Accordingly - I think, if we wanted to filter the settings, that would be a bigger and separate issue.

Are you happy for me to update this issue to just correct the issue at hand, writing something readable to the xml for TestParametersDictionary? And anything relating the filtering which settings are written out to be a separate issue?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 17, 2017

Member

I didn't mean "using" in that sense, more like "capable of using." So AFAIC there's no separate issue and I have no particular investment in this one. If I were doing it, I'd skip the legacy setting, but it's not a deal-breaker.

Member

CharliePoole commented Aug 17, 2017

I didn't mean "using" in that sense, more like "capable of using." So AFAIC there's no separate issue and I have no particular investment in this one. If I were doing it, I'd skip the legacy setting, but it's not a deal-breaker.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 21, 2017

Member

Marking as up-for-grabs.

To clarify the issue:

Currently using the console flag --params 1=d;2=e is written to the test result xml as

<setting name="TestParametersDictionary" value="System.Collections.Generic.Dictionary`2[System.String,System.String]" />

This should be written in a more readable format. Ideally, the implementation will be more generic than using the specific setting name, so any future string dictionary settings are also written out in a human-readable format.

Member

ChrisMaddock commented Aug 21, 2017

Marking as up-for-grabs.

To clarify the issue:

Currently using the console flag --params 1=d;2=e is written to the test result xml as

<setting name="TestParametersDictionary" value="System.Collections.Generic.Dictionary`2[System.String,System.String]" />

This should be written in a more readable format. Ideally, the implementation will be more generic than using the specific setting name, so any future string dictionary settings are also written out in a human-readable format.

@grimmi

This comment has been minimized.

Show comment
Hide comment
@grimmi

grimmi Aug 22, 2017

Contributor

I'd like to have a go at this. Is there any preference as to how the dictionary should be stringified? I was thinking something like [keyx, valuex], [keyz, valuez], .... Or should it be the same as the other option, i. e. keyx=valuex;keyz=valuez?

As I see it, all that's needed is to check if value is IDictionary and then iterate through the keys, constructing the readable string, right?

Contributor

grimmi commented Aug 22, 2017

I'd like to have a go at this. Is there any preference as to how the dictionary should be stringified? I was thinking something like [keyx, valuex], [keyz, valuez], .... Or should it be the same as the other option, i. e. keyx=valuex;keyz=valuez?

As I see it, all that's needed is to check if value is IDictionary and then iterate through the keys, constructing the readable string, right?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 22, 2017

Member

I think it should be the same as other key/value pairs but should be indentednested under the actual setting. That will avoid confusion since it's at least theoretically possible for a run parameter to have the same name as a built-in framework setting.

Member

CharliePoole commented Aug 22, 2017

I think it should be the same as other key/value pairs but should be indentednested under the actual setting. That will avoid confusion since it's at least theoretically possible for a run parameter to have the same name as a built-in framework setting.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 22, 2017

Member

@grimmi - Great! @rprouse, could you send a contributors invite? 👍

Is there any preference as to how the dictionary should be stringified?

That's a good question, I'm sure people will have a preference!

I don't think the keyx=valuex;keyz=valuez would be a good idea. That was our original format - the reason we have a second format is that users wished to pass keys containing semi-colons and equals symbols (e.g. database connection strings) - these of course can't be expressed in that format!

I would have suggested to use the format that we use to write dictionaries out to error results (see MsgUtils.cs) - however, that's based around angle brackets, so wouldn't be very readable in the xml! Out of the options, I think I most prefer the square brackets format that you've suggested.

@nunit/framework-team - anyone want to weigh in on this?

As I see it, all that's needed is to check if value is IDictionary and then iterate through the keys, constructing the readable string, right?

Yep - sounds about right. You should find there are already xml-writing utility methods, which will take care of escaping any necessary characters. And of course, a unit test or two. :-p

Member

ChrisMaddock commented Aug 22, 2017

@grimmi - Great! @rprouse, could you send a contributors invite? 👍

Is there any preference as to how the dictionary should be stringified?

That's a good question, I'm sure people will have a preference!

I don't think the keyx=valuex;keyz=valuez would be a good idea. That was our original format - the reason we have a second format is that users wished to pass keys containing semi-colons and equals symbols (e.g. database connection strings) - these of course can't be expressed in that format!

I would have suggested to use the format that we use to write dictionaries out to error results (see MsgUtils.cs) - however, that's based around angle brackets, so wouldn't be very readable in the xml! Out of the options, I think I most prefer the square brackets format that you've suggested.

@nunit/framework-team - anyone want to weigh in on this?

As I see it, all that's needed is to check if value is IDictionary and then iterate through the keys, constructing the readable string, right?

Yep - sounds about right. You should find there are already xml-writing utility methods, which will take care of escaping any necessary characters. And of course, a unit test or two. :-p

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 22, 2017

Member

@CharliePoole - sorry, we posted at the same time. Can you clarify what you're suggesting? Something like this?

<setting name="TestParametersDictionary">
    <param name="key1" value="value1" />
    <param name="key2" value="value2" />
</setting>

Writing that out, I'm guessing that's not what your suggesting, but I'm still not sure what you mean. 🙂

Member

ChrisMaddock commented Aug 22, 2017

@CharliePoole - sorry, we posted at the same time. Can you clarify what you're suggesting? Something like this?

<setting name="TestParametersDictionary">
    <param name="key1" value="value1" />
    <param name="key2" value="value2" />
</setting>

Writing that out, I'm guessing that's not what your suggesting, but I'm still not sure what you mean. 🙂

@rprouse

This comment has been minimized.

Show comment
Hide comment
@rprouse

rprouse Aug 22, 2017

Member

@grimmi I have invited you to the contributors team so we can assign the issue to you. Once you accept, feel free to assign this issue to yourself, or one of us can do it. Thanks for your help.

Member

rprouse commented Aug 22, 2017

@grimmi I have invited you to the contributors team so we can assign the issue to you. Once you accept, feel free to assign this issue to yourself, or one of us can do it. Thanks for your help.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 22, 2017

Member

Something like that. The element can't be <param> since that implies special knowledge of what is in the dictionary (parameters). Maybe `entry' would be more general.

The drawback of this idea is that now <setting> may contain a value or may have inner XML that expresses the value. Can you give an example of how you were thinking it might be done?

Member

CharliePoole commented Aug 22, 2017

Something like that. The element can't be <param> since that implies special knowledge of what is in the dictionary (parameters). Maybe `entry' would be more general.

The drawback of this idea is that now <setting> may contain a value or may have inner XML that expresses the value. Can you give an example of how you were thinking it might be done?

@grimmi

This comment has been minimized.

Show comment
Hide comment
@grimmi

grimmi Aug 23, 2017

Contributor

@rprouse I get an "Invitation not found" error after following the link in the invitation email. Did it expire or am I doing something wrong?

Contributor

grimmi commented Aug 23, 2017

@rprouse I get an "Invitation not found" error after following the link in the invitation email. Did it expire or am I doing something wrong?

@rprouse

This comment has been minimized.

Show comment
Hide comment
@rprouse

rprouse Aug 23, 2017

Member

@grimmi sorry, my mistake. Please try again.

Member

rprouse commented Aug 23, 2017

@grimmi sorry, my mistake. Please try again.

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 23, 2017

Member

The drawback of this idea is that now may contain a value or may have inner XML that expresses the value. Can you give an example of how you were thinking it might be done?

This was the reason I didn't initially suggest nested xml - as parsers not expecting it may have issues. I think it might be the 'most accurate' solution however - ideally we want the xml to be clear that this is passed as a dictionary, and not a string.

To keep the setting value as just a string, I was initially considering what @grimmi suggested.

<setting name="TestParametersDictionary" value="[keyx, valuex], [keyz, valuez]" />

However, I'm now thinking nested xml may be the better solution. I quite like entry.

So are we in agreement on the below format?

<setting name="TestParametersDictionary">
    <entry name="key1" value="value1" />
    <entry name="key2" value="value2" />
</setting>
Member

ChrisMaddock commented Aug 23, 2017

The drawback of this idea is that now may contain a value or may have inner XML that expresses the value. Can you give an example of how you were thinking it might be done?

This was the reason I didn't initially suggest nested xml - as parsers not expecting it may have issues. I think it might be the 'most accurate' solution however - ideally we want the xml to be clear that this is passed as a dictionary, and not a string.

To keep the setting value as just a string, I was initially considering what @grimmi suggested.

<setting name="TestParametersDictionary" value="[keyx, valuex], [keyz, valuez]" />

However, I'm now thinking nested xml may be the better solution. I quite like entry.

So are we in agreement on the below format?

<setting name="TestParametersDictionary">
    <entry name="key1" value="value1" />
    <entry name="key2" value="value2" />
</setting>
@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 23, 2017

Member

@ChrisMaddock Let's go that way then!

Member

CharliePoole commented Aug 23, 2017

@ChrisMaddock Let's go that way then!

@grimmi

This comment has been minimized.

Show comment
Hide comment
@grimmi

grimmi Aug 24, 2017

Contributor

Okay, I think I got it.
52efbd3 is the commit with changed behavior,
f5f2d9d and faa9072 added and cleaned up some tests.

What's the next step? Create a PR? Against which branch?

Contributor

grimmi commented Aug 24, 2017

Okay, I think I got it.
52efbd3 is the commit with changed behavior,
f5f2d9d and faa9072 added and cleaned up some tests.

What's the next step? Create a PR? Against which branch?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 24, 2017

Member

Against master... we'll review the code in the PR, where it's easier to add comments.

Member

CharliePoole commented Aug 24, 2017

Against master... we'll review the code in the PR, where it's easier to add comments.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

I prefer the more generic term item instead of entry because it's the term used by all the collection types including dictionaries, and I prefer key instead of name because key is the generic term used by all dictionary types while name implies special knowledge that these are parameter names.

<setting name="TestParametersDictionary">
    <item key="key1" value="value1" />
    <item key="key2" value="value2" />
</setting>

One more thing- that is fine if the property is an IDictionary<string, string>, but if I recall correctly and it's an IDictionary<string, object>, we don't want the value to be an attribute in case a value itself is structured. That brings me to the final suggestion:

<setting name="TestParametersDictionary">
    <item key="key1">value1</item>
    <item key="key2">value2</item>
</setting>

This is quite appealing because if we ever have non-associative collections, we get this:

<setting name="SomeList">
    <item>value1</item>
    <item>value2</item>
</setting>

What do you think?

Contributor

jnm2 commented Aug 25, 2017

I prefer the more generic term item instead of entry because it's the term used by all the collection types including dictionaries, and I prefer key instead of name because key is the generic term used by all dictionary types while name implies special knowledge that these are parameter names.

<setting name="TestParametersDictionary">
    <item key="key1" value="value1" />
    <item key="key2" value="value2" />
</setting>

One more thing- that is fine if the property is an IDictionary<string, string>, but if I recall correctly and it's an IDictionary<string, object>, we don't want the value to be an attribute in case a value itself is structured. That brings me to the final suggestion:

<setting name="TestParametersDictionary">
    <item key="key1">value1</item>
    <item key="key2">value2</item>
</setting>

This is quite appealing because if we ever have non-associative collections, we get this:

<setting name="SomeList">
    <item>value1</item>
    <item>value2</item>
</setting>

What do you think?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 25, 2017

Member

Entry makes sense because this special handling applies to dictionaries, not to any other type of collection.

Given that we only allow string values as parameters, I don't see a problem with value as an attribute. We have actually had a fair bit of discussion about allowing other types, and decided not to do it.

Member

CharliePoole commented Aug 25, 2017

Entry makes sense because this special handling applies to dictionaries, not to any other type of collection.

Given that we only allow string values as parameters, I don't see a problem with value as an attribute. We have actually had a fair bit of discussion about allowing other types, and decided not to do it.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

@CharliePoole

They are called items or elements but not entries in the IDictionary<,> API.
I think "entry" just comes from the old nongeneric DictionaryEntry type name which is an implementation detail that was replaced by KeyValuePair<,> once we got generic IEnumerable.

Going with value as an attribute forces a breaking change if we ever change our minds. Going with value as element contents leaves all options open, and even if we never put anything but strings in our IDictionary<string, object>, I still think it's more idomatic XML. Subjective of course. 😄

Contributor

jnm2 commented Aug 25, 2017

@CharliePoole

They are called items or elements but not entries in the IDictionary<,> API.
I think "entry" just comes from the old nongeneric DictionaryEntry type name which is an implementation detail that was replaced by KeyValuePair<,> once we got generic IEnumerable.

Going with value as an attribute forces a breaking change if we ever change our minds. Going with value as element contents leaves all options open, and even if we never put anything but strings in our IDictionary<string, object>, I still think it's more idomatic XML. Subjective of course. 😄

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 25, 2017

Member

Idiomatic XML! An oxymoron if I ever heard one!

Member

CharliePoole commented Aug 25, 2017

Idiomatic XML! An oxymoron if I ever heard one!

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 25, 2017

Contributor

Hey, it's a thing, XML isn't evil and it is a language :D The X is for eXtensible, which our value serialization won't be if we stay with the attribute, unless we later tack on multiple attributes and contents instead of using a single XML idiom. 😈

Contributor

jnm2 commented Aug 25, 2017

Hey, it's a thing, XML isn't evil and it is a language :D The X is for eXtensible, which our value serialization won't be if we stay with the attribute, unless we later tack on multiple attributes and contents instead of using a single XML idiom. 😈

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 26, 2017

Contributor

For reasons I outlined above, I have a strong preference for three changes to be made:

<setting name="TestParametersDictionary">
    <item key="key1">value1</item>
    <item key="key2">value2</item>
</setting>

Does anyone not want this syntax?

Contributor

jnm2 commented Aug 26, 2017

For reasons I outlined above, I have a strong preference for three changes to be made:

<setting name="TestParametersDictionary">
    <item key="key1">value1</item>
    <item key="key2">value2</item>
</setting>

Does anyone not want this syntax?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 26, 2017

Member

We can talk it out but your question is very odd since two of us have already expressed a different preference.

Member

CharliePoole commented Aug 26, 2017

We can talk it out but your question is very odd since two of us have already expressed a different preference.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 26, 2017

Contributor

You didn't say you disliked it, only that you didn't see a reason to change which I do. I was hoping Chris, Rob, Mikkel, Wheeler, etc. would weigh in.

Contributor

jnm2 commented Aug 26, 2017

You didn't say you disliked it, only that you didn't see a reason to change which I do. I was hoping Chris, Rob, Mikkel, Wheeler, etc. would weigh in.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 26, 2017

Member

If you consider the naming of this element as so important that it takes a discussion of every team member to decide, then we should block the issue while you call for that discussion. But in so doing I would expect you to summarize the various positions accurately and fairly.

@ChrisMaddock is the team member who has been guiding this, so I'll leave it to him to resolve what seems to me like a fairly minor issue in itself. My preference is still 'entry' but I can see using 'key' rather than 'name'.

Member

CharliePoole commented Aug 26, 2017

If you consider the naming of this element as so important that it takes a discussion of every team member to decide, then we should block the issue while you call for that discussion. But in so doing I would expect you to summarize the various positions accurately and fairly.

@ChrisMaddock is the team member who has been guiding this, so I'll leave it to him to resolve what seems to me like a fairly minor issue in itself. My preference is still 'entry' but I can see using 'key' rather than 'name'.

@rprouse rprouse added this to the 3.8 milestone Aug 26, 2017

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 26, 2017

Contributor

If you consider the naming of this element as so important that it takes a discussion of every team member to decide, then we should block the issue while you call for that discussion. But in so doing I would expect you to summarize the various positions accurately and fairly.

It's important to me because the change is easily made now and all but impossible later. Re: fair and accurate, I'm sorry, that is certainly my intent:

  1. I don't want us to use name because this same code will be run for dictionaries whose keys aren't necessarily names. key seems to be the BCL term used. @CharliePoole is okay with this.

  2. @CharliePoole has a preference for <entry>. I have a preference for <item> which isn't terribly strong but I do like the symmetry if we ever serialize list items. MSDN refers to them as elements rather than items or entries.

  3. It seems nicer to me for the value to be the contents of the <entry>/<item> element rather than a value attribute. Unlike the attribute, it leaves our options open if we ever put a non-string value in our IDictionary<string, object> and want to serialize it with a structure. Also unlike the attribute, it has a symmetry with the syntax I'd expect if we serialized a list: <item>A</item><item>B</item>.
    @CharliePoole does not see a problem with keeping it as an attribute because in prior discussions we decided not to allow anything but strings.

    (@CharliePoole that is true, we decided test parameters would only be passed in as strings, but this code is going to run any time an IDictionary turns up. If any other setting ever becomes a dictionary– though for some reason we chose IDictionary<string, object> over IDictionary<string, string> for test parameters– it will have to abide by the rules we set here so I wouldn't impose test parameter rules on all dictionary-like settings.)

Contributor

jnm2 commented Aug 26, 2017

If you consider the naming of this element as so important that it takes a discussion of every team member to decide, then we should block the issue while you call for that discussion. But in so doing I would expect you to summarize the various positions accurately and fairly.

It's important to me because the change is easily made now and all but impossible later. Re: fair and accurate, I'm sorry, that is certainly my intent:

  1. I don't want us to use name because this same code will be run for dictionaries whose keys aren't necessarily names. key seems to be the BCL term used. @CharliePoole is okay with this.

  2. @CharliePoole has a preference for <entry>. I have a preference for <item> which isn't terribly strong but I do like the symmetry if we ever serialize list items. MSDN refers to them as elements rather than items or entries.

  3. It seems nicer to me for the value to be the contents of the <entry>/<item> element rather than a value attribute. Unlike the attribute, it leaves our options open if we ever put a non-string value in our IDictionary<string, object> and want to serialize it with a structure. Also unlike the attribute, it has a symmetry with the syntax I'd expect if we serialized a list: <item>A</item><item>B</item>.
    @CharliePoole does not see a problem with keeping it as an attribute because in prior discussions we decided not to allow anything but strings.

    (@CharliePoole that is true, we decided test parameters would only be passed in as strings, but this code is going to run any time an IDictionary turns up. If any other setting ever becomes a dictionary– though for some reason we chose IDictionary<string, object> over IDictionary<string, string> for test parameters– it will have to abide by the rules we set here so I wouldn't impose test parameter rules on all dictionary-like settings.)

@mikkelbu

This comment has been minimized.

Show comment
Hide comment
@mikkelbu

mikkelbu Aug 26, 2017

Member

I agree with @jnm2 on this. Especially, the third point.

Member

mikkelbu commented Aug 26, 2017

I agree with @jnm2 on this. Especially, the third point.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 26, 2017

Member

I'm bowing out of this discussion. I dislike that it has become a free-for-all. @ChrisMaddock was the guy shepherding this along and was doing a good job as far as I'm concerned. Having everybody pile on rather than letting him resolve the opposing points of view seems wrong to me.

Of course, since we don't designate the team member who is mentoring a new contributor and shepherding the issue to completion in any formal way, folks may not have read far enough back in the discussion to see it. I'd like to re-propose the notion that some team member should always self-assign themselves to the issue along with the contributor working on it. @rprouse You didn't want to do that in the past, but it would provide a bit of clarity and help any team members wanting to chime in by letting them see who is already involved.

Member

CharliePoole commented Aug 26, 2017

I'm bowing out of this discussion. I dislike that it has become a free-for-all. @ChrisMaddock was the guy shepherding this along and was doing a good job as far as I'm concerned. Having everybody pile on rather than letting him resolve the opposing points of view seems wrong to me.

Of course, since we don't designate the team member who is mentoring a new contributor and shepherding the issue to completion in any formal way, folks may not have read far enough back in the discussion to see it. I'd like to re-propose the notion that some team member should always self-assign themselves to the issue along with the contributor working on it. @rprouse You didn't want to do that in the past, but it would provide a bit of clarity and help any team members wanting to chime in by letting them see who is already involved.

@grimmi

This comment has been minimized.

Show comment
Hide comment
@grimmi

grimmi Aug 26, 2017

Contributor

So, is there consensus (or any decision) as to what the XML should look like? I'm a bit lost here 😉

Contributor

grimmi commented Aug 26, 2017

So, is there consensus (or any decision) as to what the XML should look like? I'm a bit lost here 😉

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 26, 2017

Contributor

I see I plunged in without understanding the process and expectations. I'm sorry for that and as always I trust the team- I'm positive that Chris as the shepherd and Rob as the team lead will make sure the issue is in a good place!

Contributor

jnm2 commented Aug 26, 2017

I see I plunged in without understanding the process and expectations. I'm sorry for that and as always I trust the team- I'm positive that Chris as the shepherd and Rob as the team lead will make sure the issue is in a good place!

@rprouse

This comment has been minimized.

Show comment
Hide comment
@rprouse

rprouse Aug 26, 2017

Member

I'd like to re-propose the notion that some team member should always self-assign themselves to the issue along with the contributor working on it.

I think that is a good idea going forward. I actually don't remember saying I didn't want to do it in the past, but if I did, I think it is a good idea now. I have also been thinking that we should do the same with PRs so that one team member takes the lead.

Member

rprouse commented Aug 26, 2017

I'd like to re-propose the notion that some team member should always self-assign themselves to the issue along with the contributor working on it.

I think that is a good idea going forward. I actually don't remember saying I didn't want to do it in the past, but if I did, I think it is a good idea now. I have also been thinking that we should do the same with PRs so that one team member takes the lead.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Aug 26, 2017

Member

PRs make sense too. Usually, but not always, I would expect the same person who is shepherding the issue to continue with the PR.

As far as past discussions go, it's possible that our transitioning roles led to confusion between us. I made the suggestion and waited for you to either do it or tell me to go ahead and implement it.

Member

CharliePoole commented Aug 26, 2017

PRs make sense too. Usually, but not always, I would expect the same person who is shepherding the issue to continue with the PR.

As far as past discussions go, it's possible that our transitioning roles led to confusion between us. I made the suggestion and waited for you to either do it or tell me to go ahead and implement it.

rprouse added a commit that referenced this issue Aug 27, 2017

Merge pull request #2382 from grimmi/dev-2298
Write TestParametersDictionary to xml result file in readable format (#2298)

@rprouse rprouse added the closed:done label Aug 27, 2017

@ChrisMaddock

This comment has been minimized.

Show comment
Hide comment
@ChrisMaddock

ChrisMaddock Aug 28, 2017

Member

Sorry, I've been offline for a couple of days. Great to see this merged - thanks for figuring it out everyone, and thanks @grimmi for writing the code! 😄

Member

ChrisMaddock commented Aug 28, 2017

Sorry, I've been offline for a couple of days. Great to see this merged - thanks for figuring it out everyone, and thanks @grimmi for writing the code! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment