Adding support for local metadata on properties. #2320
Conversation
There was a problem hiding this comment.
Minor nitpick regarding indentation. Do you have editorconfig installed? Otherwise, this LGTM. 👍
| @@ -117,87 +117,87 @@ public void CountsShouldContainKeyAndCountBe(string key, int count) | |||
| } | |||
|
|
|||
| #pragma warning disable 618 | |||
| public void Visit(StringProperty mapping) | |||
| public void Visit(IStringProperty mapping) | |||
There was a problem hiding this comment.
Nice catch on these not being interfaces! 👍
There was a problem hiding this comment.
Cool! Glad you are ok with that change.
|
I will fix the indentation. |
|
Ok, I just corrected all the whitespace issues. |
|
LGTM |
|
@ejsmith LGTM. One last request before we pull this in -- would you mind squashing these to a single commit? |
|
I thought GitHub added that ability to pull requests? That's why I didn't bother doing it. |
|
You can do that when you merge the Pr via GitHub it's a drop down option |
|
Oh nice! That's awesome, I wasn't aware that they added this. |
|
You Can also change the branch you merge to :) |
Yea that one I know of. The new merge features flew past my radar though :) |
* Adding support for local metadata on properties. * Fix whitespace * Move LocalMetadata into the IProperty interface Closes #2320
| { | ||
| public class LocalMetadataTests | ||
| { | ||
| [Fact] |
There was a problem hiding this comment.
Should be [U], in line with other unit tests
Initial pull request for adding support for local metadata to properties. I had to change the IMappingVisitor to use the property interfaces since it was taking the concrete classes and the mapping descriptors would not work with this. That doesn't seem to break anything and seems to just make the visitor support more scenarios, but I also realize that it's a breaking change that you might not want to take on and we could easily create our own visitor.