Skip to content

Conversation

@sungam3r
Copy link
Member

@sungam3r sungam3r commented Jan 8, 2026

fixes #171

@github-actions github-actions bot added the tests Pull request that adds new or changes existing tests label Jan 8, 2026
Copy link

@HaydenReeve HaydenReeve left a 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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise

Oh this is an excellent way to handle this; I like this.

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!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought(non-blocking)

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AllowDestructuringOnlyExplicitlyMarkedProperties -> AllowOnlyExplicitlyMarkedProperties or AllowOnlyExplicitlyMarked

?

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.

@sungam3r sungam3r merged commit 27d4538 into master Jan 11, 2026
8 checks passed
@sungam3r sungam3r deleted the filter branch January 11, 2026 17:49
@codecov
Copy link

codecov bot commented Jan 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (fe96bfe) to head (ef329ba).
⚠️ Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Pull request that adds new or changes existing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Destructure only explicitly marked properties

3 participants