Skip to content
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

MudDataGrid: Add ICloneStrategy #8851

Merged
merged 7 commits into from
May 5, 2024
Merged

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented May 2, 2024

Description

Resolves: #8848
Read my two comments for details: #8848 (comment), #8848 (comment)

In short: This can kill two birds with one stone.

  1. If they provide a class that STJ cannot resolve - there is abstraction.
  2. If they hit AOT/trimming issues - there is abstraction, either provide a trim safe implementation or use STJ with provided JsonSerializerContext.

Two built in resolvers: STJ without any custom configuration (default), ICloneable (not sure if it makes sense to provide second option when its easy to implement by other parties, but i thought I can use it in test as an showcase, tho I could just add it in test project)

How Has This Been Tested?

Added new bUnit tests

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@ScarletKuro ScarletKuro requested a review from henon May 2, 2024 18:10
@github-actions github-actions bot added enhancement New feature or request PR: needs review labels May 2, 2024
@ScarletKuro
Copy link
Member Author

@henon will wait your opinion on that.

Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.13%. Comparing base (28bc599) to head (f72e05b).
Report is 142 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8851      +/-   ##
==========================================
+ Coverage   89.82%   90.13%   +0.30%     
==========================================
  Files         412      423      +11     
  Lines       11878    12309     +431     
  Branches     2364     2431      +67     
==========================================
+ Hits        10670    11095     +425     
+ Misses        681      665      -16     
- Partials      527      549      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henon
Copy link
Collaborator

henon commented May 3, 2024

Makes sense of course. This is an application of the Strategy Pattern, shall we name it ICloneStrategy<T> or IDeepCopyStrategy<T> instead?

@ScarletKuro
Copy link
Member Author

ICloneStrategy

I like this name, I will rename it later and do tests. I guess the explanation that it has to be a deep clone and not shallow one can be achieved via xmldocs.

@ScarletKuro
Copy link
Member Author

Renamed to ICloneStrategy and moved to Utilities (maybe in future something else will need to deep clone object and this can be reused).
Added two tests.
One that shows using the non default strategy and that it works with abstract classes and second test shows that the default one will fail if no polymorphic type discriminators are present for STJ (or additional configuration which would require to implement custom strategy)

@ScarletKuro ScarletKuro changed the title MudDataGrid: Add IDeepCopyResolver MudDataGrid: Add ICloneStrategy May 4, 2024
@ScarletKuro
Copy link
Member Author

I need tmr to tweak the xml doc, so do not merge.
But review is welcome.

@henon henon marked this pull request as draft May 4, 2024 07:11
@henon
Copy link
Collaborator

henon commented May 4, 2024

OK, converted to draft for now

@ScarletKuro ScarletKuro marked this pull request as ready for review May 4, 2024 09:47
@ScarletKuro
Copy link
Member Author

ready for merge

@henon henon merged commit 33fc0ad into MudBlazor:dev May 5, 2024
4 checks passed
@henon
Copy link
Collaborator

henon commented May 5, 2024

Awesome. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGrid edit not working when object contains abstract class
2 participants