Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Serializer will now ignore: indexer properties, and properties with their name listed in a JsonIgnore above the declaring class #299
base: main
Are you sure you want to change the base?
Serializer will now ignore: indexer properties, and properties with their name listed in a JsonIgnore above the declaring class #299
Changes from 5 commits
9c5c5c6
51f3c9b
7bc39ab
049612c
453f180
e3f541c
37207a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add an extra line between the functions, so one is missing before the attribute. Same in the next few functions. It helps for readibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for more style errors. Even though I like organized code, I'm not used to checking that aspect quite so much. Plus it was a little messy from moving stuff around to get tests working. Will be better next time, don't want to waste your valuable time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to apologize about! You're doing great! 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on José remark, you're doing great! No worry, you should have seen my very first PR on a .NET repository :-D Now, with habit, it's easy to spot them. We've been added StyleCop linter on the IoT repository as it's where we do have the most contributions. Not yet into those classes one. That will slowly come but it's not urgent. And anyway, we won't put it in place in the tests as it doesn't make too much sense.
So all good! You'll be indeed much better next time and at some point like us being able to spot those :-D
It's really to make it easier to read the code and navigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that you can as well add an error message in case it's not successful. So you know where it broke and maybe other values you want to track in the test result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you arrive here, then it's always successful. So no need to test the value of jsonSuccess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, as long as my caching solution is acceptable. I will get to those last few fixes you suggested as soon as I can. The one thing I didn't do is run the performance tests with the code before I touched it. I can do that as well and provide my findings when I do my next push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, the value will always be false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing here:
For each property you are creating gettersToIgnore based on classAttributes. Which means doing the same work for each property (foreach loop -> ShouldSerializeMethod -> ShouldIgnorePropertyFromClassAttribute -> creating array). Faster approach should be creating ignored property array before
foreach (MethodInfo method in methods)
.BTW @josesimoes does nanoFramework support attributes on properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@torbacz there is nothing preventing you from decorating properties with attributes.
I guess that you want to know if you can reach custom attributes for properties. That's a different story: that is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my question 😅 Because then we could just decorate each property. Also I'm not quite sure, if we should implement such solution. It's not resolving any problems. If you have class with properties which you don't want to send, just create derived class and pass it to JSON lib.
Don't get me wrong, I like new features but I'm concern about performance, adding new feature where there is a possibility for workaround is making lib much complicated over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for the benchmark results and then decide.
I do share your concern. Impacting performance is something to avoid at all costs. Unless the trade-off it's relevant.
As a side comment: I keep being surprised on how much interest this library has gathered from the community! And the investments on improving it are also surprising to me. 😄