Conversation
github-actions
Bot
added
the
tests
HaydenReeve
left a comment
HaydenReeve
left a comment
There was a problem hiding this comment.
A clever solution without overcomplicating things. I like it!
| properties with `[NotLogged]`. | ||
|
|
||
| ```cs | ||
| [AllowDestructuringOnlyExplicitlyMarkedProperties] |
There was a problem hiding this comment.
Oh this is an excellent way to handle this; I like this.
There was a problem hiding this comment.
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.
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.
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.
AllowDestructuringOnlyExplicitlyMarkedProperties -> AllowOnlyExplicitlyMarkedProperties or AllowOnlyExplicitlyMarked
?
There was a problem hiding this comment.
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:
|
2 participants
fixes #171