search

jehugaleahsa / mustache-sharp

An extension of the mustache text template engine for .NET.
The Unlicense
306 stars 78 forks source link

Fix for Exception if 2 properties have the same name #22

Closed nikkilocke closed 10 years ago

nikkilocke commented 10 years ago

And an additional test to test it.

nikkilocke commented 10 years ago

Now added ability to access fields as well as properties.

jehugaleahsa commented 10 years ago

When you keep adding a bunch of changes to the same merge request, it makes it impossible to merge any of your changes. I do appreciate your interest and effort, just understand it takes a lot of time to push out changes. Keep in mind, I am mid-way through a massive project at work, so by the time I get home, maintaining an open source project is pretty low on my priorities list.

After reviewing your changes, I improved the code to retrieve values from public fields. My implementation doesn't require constantly checking the type of the member. Instead I use a Func<object, object> to retrieve the property or field's value.

Additionally, your solution to handling new members was simple and straight-forward, but I couldn't find any documentation supporting the assumption that properties are returned in most-derived-first order. I had to write some "interesting" code to correctly determine the most derived version of a member. I'd gladly switch back to your simple implementation if you can find some C# standard or MSDN reference that says it's a safe assumption.

As for the other changes you have made, I haven't decided whether they are features I want to include in Mustache# yet. It takes time for me to review your changes and to make sure they are adequately tested and, more importantly, fit nicely with the project vision. Right now, I am interested in array indexes and keeping newlines. As for #unless, I don't see that getting added any time soon. I have some plans for improving the ConditionalTag class in the near future to make deriving from it easier.

I am not sure how to merge your other changes, so I am going to close this merge request. I would recommend refreshing your branch with the latest code. Once/if you fix your merge conflicts, I would recommend submitting each of your changes as a separate merge request. It will keep me from feeling overwhelmed and hopefully help me to get code out sooner. Thanks again for putting so much time into this!

nikkilocke commented 10 years ago

Thanks for taking this on. I'm sorry multiple changes in my pull request was the wrong way to do it. I am very unfamiliar with git, and believed the hype that it was really good at separating and merging different changes. My fix for new members was just intended to stop the exceptions being thrown. There was never intended to be any guarantee that you would get the most derived property, although it seems to do so. I agree that code to ensure this is a good idea, and sorry it was "interesting"! I'm glad you also improved the public fields code - I will be checking out what you have done and learning from it. I expect you will also find a better way to do array indexes - I imagine there is a better way, but obviously I don't have your knowledge of the project vision, or overview of the architecture. The newlines code is pretty trivial, although you may want to choose a different way of activating it. I agree about improvements to ConditionalTag - I wanted to allow unless... else, but couldn't figure out a way to do it without major surgery. When I next have some spare time, I will try to produce separate pull requests for each change. I'm pretty busy myself right now, so it may be a while. Thanks again for a great library, and all the effort you put in to maintain and improve it.

nikkilocke commented 10 years ago

Hi Travis,

I have created a new pull request for the preserve newlines change (it is very simple, so should be easy to review and merge).

I am not sure how to create a separate merge request for array access, so will do a new one when you have had time to look at this one.

Best regards,

Nikki

Nikki Locke, Trumphurst Ltd. PC & Unix consultancy & programming http://www.trumphurst.com/

nikkilocke commented 10 years ago

Hi Travis,

I have checked the array access stuff into my fork, but have no idea how to create a separate pull request, sorry.

Regards,

Nikki

Nikki Locke, Trumphurst Ltd. PC & Unix consultancy & programming http://www.trumphurst.com/