-
Notifications
You must be signed in to change notification settings - Fork 36
Introduce IPropertyFilterAttribute #215
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
Conversation
src/Destructurama.Attributed.Tests/Approval/Destructurama.Attributed.approved.txt
Outdated
Show resolved
Hide resolved
src/Destructurama.Attributed.Tests/Approval/Destructurama.Attributed.approved.txt
Outdated
Show resolved
Hide resolved
HaydenReeve
left a comment
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.
A clever solution without overcomplicating things. I like it!
| properties with `[NotLogged]`. | ||
|
|
||
| ```cs | ||
| [AllowDestructuringOnlyExplicitlyMarkedProperties] |
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.
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 was going to make a few suggestions, but honestly, I am a fan of the simplicity.
There's no reason I can think of straight away that would require the additional complexity used by other similar libraries which have [Destructure(Filter.OptIn)] and [Destructure(Filter.OptOut)] syntax styles; unless a third style was needed.
This will resolve #171 for me. Thanks!
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.
Thinking some more on this, I can see a downside of the new property being extremely verbose, but at least on the pro-side it’s quite clear and explicit.
[OnlyDestructureMarkedProperties] or [DestructureMarkedPropertiesOnly] might be an option for something more concise but still convey the correct meaning? It’s not as perfectly exact, but it’s less wordy and easier to read (for me at least).
Another option to consider might be
[NotLoggedByDefault] which plays on the [NotLogged] intuitive terminology used elsewhere.
Otherwise by taking in an argument and opening it for extension, you could go with
[Logged(DestructuramaFilter.Always)] and [Logged(DestructuramaFilter.OnlyExplicitAttributes)] or something to this effect which allows you to extend it later without introducing breaking changes.
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.
DestructuramaFilter.Always should be compile-time constant.
allows you to extend it later without introducing breaking changes
New interface allows you (as client, not me as author) to extend behaviour in any way.
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.
AllowDestructuringOnlyExplicitlyMarkedProperties -> AllowOnlyExplicitlyMarkedProperties or AllowOnlyExplicitlyMarked
?
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.
Out of the three I prefer the first one because it explicitly mentioned the Destructuring key word. I think that makes it more discoverable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #215 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 13 +1
Lines 259 264 +5
Branches 40 41 +1
=========================================
+ Hits 259 264 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fixes #171