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

✨ [FEATURE] Add 'Enum.AsNoTracking' to all methods instead of using bool values #16

Open
jeffward01 opened this issue Sep 26, 2022 · 4 comments · May be fixed by #19
Open

✨ [FEATURE] Add 'Enum.AsNoTracking' to all methods instead of using bool values #16

jeffward01 opened this issue Sep 26, 2022 · 4 comments · May be fixed by #19
Assignees
Labels
enhancement New feature or request

Comments

@jeffward01
Copy link

Summary

I would like to add an overload that takes Enum.AsNoTracking to all of the methods. For example, currently there might be a method such as this:

image

I would like to add an overload that takes an enum instead of bool

image

In this enhancement I suggest we:

  • Add the overload parameter, and eventually in later versions remove the bool parameter
  • Add the overload parameter and also remove the bool parameter

What do you suggest?

Advantage

Explicit parameters help the developer. using a bool of true or false can mean anything. The developer must look at the xml notes in order to see "Ah yes... this is for change tracking"

Here are some articles about it:

Additional context
I will need your advice and suggestions on naming the classes and properties.

I suggest these, please let me know your input and feel free to rename or re-design

Enum

public enum EfTrackingOptions
{
        
    /// <summary>
    /// Disables EfCore change tracking
    /// </summary>
    AsNoTracking,
            
    /// <summary>
    /// Enables EfCore Change Tracking
    /// </summary>
    WithTracking
}

Extension Method

internal static class EfTrackingOptionExtensions
{
    public static bool HasNoTracking(this EfTrackingOptions options)
    {
        return options == EfTrackingOptions.AsNoTracking;
    }
}

Outstanding Questions & Input required

  1. Should we do option 1 of add the overload, and not remove the bool, or option 2 of add the overload and remove the bool parameter
  2. Naming suggestions of enum class name and property names
  3. Naming suggestions of extension method class name and method name
@jeffward01 jeffward01 added the enhancement New feature or request label Sep 26, 2022
@furkandeveloper
Copy link
Owner

First of all, thank you for this great suggestion.
Your recent contributions to EasyRepository.EFCore has been very impressive.
It would certainly be much more readable and understandable to use enum instead of bool.
I will be looking forward to your work on this.

@jeffward01

@jeffward01
Copy link
Author

jeffward01 commented Sep 26, 2022

@furkandeveloper - Its my pleasure! I have a bunch of libraries I create myself for my own development, and I needed a really good library for Generic Repository (I love repository pattern, many people say its not needed and overkill, I say to these people, obviously they have not had to work on large enterprise code bases before 🙃 )

I found your library, and its great! there are some things that I wanted to change and improve, and thank you for being so receptive to them.

I have a few more changes / features I plan to suggest after I have more usage with your library, specifically around the IIncludableQueryable and perhaps an extension nuget package to incorporate IncludeFilter - I will open a feature request when i have a better idea for this feature. its a very 'rough' idea now.


I need to thank you as well for being so responsive and active on GitHub! I really appreciate it!

@jeffward01
Copy link
Author

jeffward01 commented Sep 26, 2022

In regards to:

In this enhancement I suggest we:

1.) Add the overload parameter, and eventually in later versions remove the bool parameter
2.) Add the overload parameter and also remove the bool parameter

What do you suggest?

What are your thoughts on this, currently I am just adding the overload method. (option 1)

Personally, i'd like to remove the bool parameter entirely, but I also know that this is a "breaking" change, and perhaps its better to add the [Obsolete] attribute to it until 3.0.0 is released? or perhaps 2.5.0,

I know that this is one of the most sensitive parts when it comes to maintaining libraries

@furkandeveloper
Copy link
Owner

In regards to:

In this enhancement I suggest we:
1.) Add the overload parameter, and eventually in later versions remove the bool parameter
2.) Add the overload parameter and also remove the bool parameter
What do you suggest?

What are your thoughts on this, currently I am just adding the overload method. (option 1)

Personally, i'd like to remove the bool parameter entirely, but I also know that this is a "breaking" change, and perhaps its better to add the [Obsolete] attribute to it until 3.0.0 is released? or perhaps 2.5.0,

I know that this is one of the most sensitive parts when it comes to maintaining libraries

Definitely old methods should continue to be supported until 3.0.0 is released.

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 a pull request may close this issue.

2 participants