Handle different type property name case #39
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 99.37% 99.41% +0.03%
==========================================
Files 3 3
Lines 161 170 +9
==========================================
+ Hits 160 169 +9
Misses 1 1
Continue to review full report at Codecov.
|
manuc66
changed the title
|
|
||
|
|
||
| var persons = JsonConvert.DeserializeObject<ICollection<Person>>(json); | ||
| Assert.AreEqual("Painter", (persons.Last() as Artist)?.Skill); |
There was a problem hiding this comment.
I'm suggesting to add yet another Artist with Skill to show that both cases are valid
|
You can also use predefined phrases such as |
|
SonarQube analysis reported 3 issues Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
|
Well, Sonar is right, it's not good idea to update static fields from instance methods. |
|
@Pzixel I would agree but those fields are thread static and it's the only "hack" that I could find to transmit information through stack calls without modifying JSON.NET. And even if it's considered bad practice in general in this particular case it's safe and tested for thread safety, see at least: If you have a better idea you're really welcome ;-) |
|
@manuc66 why not just make it instance fields? In this case it should't be threadlocal. If you consider single instance accessing from different threads just use |
|
@Pzixel it's per thread instance |
|
@manuc66 why should one instance update this value for another one? |
|
@Pzixel static field annotated with thread local means that each thread has it own value and this library "remember" that the converter has already been called on a thread so that it could delegate the current and subtree de-serialisation to JSON.NET once the subtype information has been discovered. It also handle the case where the converter in invoked in a subtree. |
|
@manuc66 thank you, I know what does it mean. I'd like to ask more high level aspect: why should one instance affect another instance in same thread? |
|
@Pzixel it shouldn't need but should not produce nasty effect since the purpose is to skip the second call to converter once it has already been called and in between there is no possibility to be in another converter instance. |
|
@manuc66 I propose to not introduce premature optimization until it's really required. |
|
@Pzixel I I assure you that the use of thread static has really not been taken inconsiderately from my side. If you think you can solve the problematic with a cleaner approach feel free to submit a PR. |
|
@manuc66 if you have some benchmark that shows that it really worth it then I have to arguments why it shouldn't be done this way. Otherwise I suggest either add them or just make it instance fields and check if it works fine. I'd like to suggest some cleaner solution for the former case too, but I don't really get the whole requirements set yet. |
|
@Pzixel The benchmark are in the test suite, if you could solve it without breaking the test suite you got it. Currently if you remove |
|
Well, if you are this sure about the solution then this warning should be ignored via some attribute or something, I guress. |
|
@Pzixel The warning popout in pull request but are marked as wont fix/false positive on sonar cloud for the master branch see: https://sonarcloud.io/project/issues?id=manuc66%3AJsonSubtypes%3Amaster&resolutions=WONTFIX&types=CODE_SMELL |
Now handle different type property name case
resolve #31
resolve #38