Bug fix: generate FieldnameWithTypeAndReference for dictionary fields.

[romkatv]

I wasn't able to run tests. Killed half an hour trying every suggestion from StackOverflow but Visual Studio still didn't see any tests.

[kbilsted]

Thanks for the pull request. Currently there are failing tests. Are you using VS with resharper 10? I have major issues myself with resharper 10 and detecting tests. Just downgraded to resharper 9 for that very reason.

[romkatv]

Hi Kasper,

Thanks for the prompt reply. In retrospect, pull request wasn't the right medium. I should've filed a feature request -- I believe it would work better. Let me try to explain what I'm doing.

I've been happily using my own formatter that produces strings like these:

{A = 0, B = {C = 1, D = 2}}

Lists look like this:

{A = ([0] = "hello", [1] = "bye")}

I'd like dictionaries to look similar to lists:

{A = ([42] = "hello", [1337] = "bye")}

I managed to get it working but it's really awkward because there is no FieldnameWithTypeAndReference token when the dictionary starts.

Does that make sense? Do you think the library could support this type of formatting?

Another and potentially bigger problem is that it's not possible to print empty dictionaries. Consider the following class definition:

class X { public Dictionary<int, int> D = new Dictionary<int, int>(); };

Introspection generates the following tokens for an instance of X with empty D:

1. FieldnameWithTypeAndReference for "X".
2. StartScope.
3. StartEnumeration.
4. EndEnumeration.
5. EndScope.

The dictionary is described with StartEnumeration and EndEnumeration, but neither of them has the the field name -- "D".

[kbilsted]

Hi @romkatv thank you for the clarification. I think it sounds reasonable. Also the json output is not really json so that need to be reworked anyway.so yeah we can just as well introduce breaking changes :)

[kbilsted]

If you still cannot run the tests feel free to upgrade the nunit framework. I've noticed that in the v3 there no longer is a message that you can supply with the assert or maybe I'm overlooking something. That may prove a challenge for the api.. or maybe a wrapper function can be exposed to wrap and ignore passing in the message parameter.

[romkatv]

I already tried upgrading to NUnit 3. Didn't help.

Perhaps it would be easier for you to update the test cases, provided that my fix makes sense and you have a way to automatically update the test cases? If not, I'll try to get tests working on my machine when I get a free minute (probably next weekend).

[kbilsted]

Try installing the ugly nunit gui runner . If I upgrade nunit I have to upgrade resharper also. And v10 suxx..

We could change it to xunit but I dislike how you have to run through hoops in order to do casual console write

[romkatv]

OK, I got it running with the nunit gui runner. Thanks for the suggestion!

The JSON and XML formatters need fixing. I'll ping you when I'm done with them. Cheers!

[kbilsted]

Great. But it's fine with me to take small incremental changes. Eg a pr per format.

I'm also considering if making all the processes more lazy could speed up things. But my experience is that it sacrifices debugging

[romkatv]

I've made a few more changes. Please take another look.

1. Split Enumeration into two distinct sequence types: List and Dict. Instead of TokenType.{Start,Stop}Enumeration we now have TokenType.{Start,Stop}{List,Dict}.
2. Split Field.SimpleKeyInArrayOrDictionary into Field.Key (for elements of a Dict) and Field.Index (for elements of a List). Coupled with (1) it makes it easier to write formatters that distinguish between lists and dicts (which is pretty much all of them).
3. Made JSON formatter more similar to JSON.
4. Made XML formatter produce less verbose output with more consistent schema.
5. Updated all tests.

[kbilsted]

hi @romkatv thanks. I'll have a look at it.Without having looked at the code I feel that the list-type you mentioned perhaps is better named as "enumerable" or something - it may be far from a list, but it is something whose elements are not organized as key,value

you also say that it is closer to JSON, what is still outstanding?

[romkatv]

you also say that it is closer to JSON, what is still outstanding?

There are a few cases where the produced output isn't valid JSON. For example, string fields may have quotes in them which don't get escaped (XML formatter has the same problem but with different symbols). Then references aren't part of the JSON spec, of course. I mean, it's good enough for ToString(), which is only meant to produce human-readable output.

There is also weird double-quoting of strings in XML format. That's not how XML usually looks like. Actually, let me just fix that.... Done.

[romkatv]

I feel that the list-type you mentioned perhaps is better named as "enumerable" or something

Right, I used 'list' in a sense it's often used in other languages, meaning a sequence. In C# it corresponds to IEnumerable. I thought that calling it StartEnumerable and EndEnumerable might be confusing because not all IEnumerable instances generate these (string and Dictionary with simple keys don't). By the way, I found StartEnumeration confusing, too -- initially I thought it's about the enums.

Anyway, that's why I chose these names but I'm not attached to them. You are the boss, let me know what you want them to be called.

[kbilsted]

@romkatv list vs enumerable: well I think I agree with you - I just felt we needed that discussion brought to the table.

[kbilsted]

Was a busy day at work today - I hope to have more energy tomorrow I'll scrutinize all your changes. Really appreciate your effort so far!

[kbilsted]

hi @romkatv I hope I haven't discouraged you with my many many comments. I think the problem is that the PR is spanning too many things perhaps.

I think it would be easier to understand and discuss if it was broken down into more PR's and with a branch for each PR. But im not sure how familiar you are with git and branching? if you are familiar with branching I'd prefer we

• create a ticket for one of the things in here,
• and then create a branch for that issue
• and then a PR for that branch.

Then creating release notes is really easy. and we can easier discuss and close each PR.

I would leave this master unchanged for now so that we don't loose all the comments. how does all this sound? too much work on your part? if so let me know.. its not like it want to prevent your changes going in ;) its rather the opposite ;)

thanks again for the huge amount of work so far.

ps this is just a minddump - since you are toying with the fundamentals - maybe it is not that easy to split up the pr.

[romkatv]

I addressed all your comments. Please take a another look.

My apologies for creating such a large PR. Seems like we are converging in our discussion. Perhaps you could accept it as a whole? I can't find the motivation for splitting it up.

[romkatv]

Thanks for reviewing and accepting the PR. It was a tough one.

When do you expect to push a new version to NuGet?

[kbilsted]

I want to rename the main class for VBA users. Also thinking about lookin into some performance tests. Your pr made one test go from 25 to 23 seconds..that's 10% :)

[kbilsted]

The configuration can do with caching the unmatched fields. Maybe you want to do that?

[romkatv]

Not right now. I'm busy with a few other projects.

[kbilsted]

Also I will create 4 issues found on the basis of your pr that we may want to solve first.will create those issues today

[kbilsted]

No problem. Many thanks for the pr. Will make a new release within a week