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

Added TryGetValue<T> for Properties - fix #21343 #21911

Merged
merged 6 commits into from
Aug 10, 2020
Merged

Added TryGetValue<T> for Properties - fix #21343 #21911

merged 6 commits into from
Aug 10, 2020

Conversation

m4ss1m0g
Copy link
Contributor

@m4ss1m0g m4ss1m0g commented Aug 3, 2020

Summary of the changes

  • Added on abstract class Microsoft.EntityFrameworkCore.ChangeTracking.PropertyValues the method public abstract bool TryGetValue<TValue>([NotNull] string propertyName, out TValue value);
  • The method implementation on derived classes
    ° CurrentProertyValues
    ° ArrayPropertyValues
    ° OriginalPropertyValues
  • Added unit test test/EFCore.Tests/ChangeTracking/Internal/PropertyValuesTest.cs

Fixes #21343

/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override bool TryGetValue<TValue>(string propertyName, out TValue value)
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be re-implemented in subclass overrides? It looks like maybe the implementation can be common and in the base class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found a base, only abstract classes. I have implemented the method on concrete classes that implement the PropertyValues abstract class. This is my first PR on this project, maybe I missed something. Can you give me some clue ? Thanks.

Copy link
Contributor Author

@m4ss1m0g m4ss1m0g Aug 4, 2020

Choose a reason for hiding this comment

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

@ajcvickers I understand you prev message, sorry I implement the code in the PropertyValues class.

@ajcvickers ajcvickers self-assigned this Aug 3, 2020
@ajcvickers
Copy link
Member

@m4ss1m0g Looking better, but tests are failing. Take a look at Getting and building the code for how you can run all the tests locally.

@m4ss1m0g
Copy link
Contributor Author

m4ss1m0g commented Aug 8, 2020

The line var property = EntityType.FindProperty(propertyName); raise the exception, I'm read properties through var property = Properties.FirstOrDefault(p=> p.Name == propertyName);

@ajcvickers ajcvickers merged commit 9346ff8 into dotnet:main Aug 10, 2020
@ajcvickers
Copy link
Member

@m4ss1m0g Thanks for the contribution!

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

Successfully merging this pull request may close these issues.

EntityEntry.CurrentValues
3 participants