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

feat(scope): make Abstractions layer #131

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mo3in
Copy link

@mo3in mo3in commented Nov 3, 2023

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes #130 (issue)

Type of change

  • 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)

Checklist

  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes

Copy link

what-the-diff bot commented Nov 3, 2023

PR Summary

  • Introduction of a New Project File
    A new project file, named Gridify.Abstractions.csproj, has been created which includes the package description and a reference to the helpful README.md file for user guidance.

  • Creation of a 'root.props' file
    A new file named root.props has been added. This file contains various essential project properties such as version number, authors, and the package license.

  • Renamed Files in the src/Gridify Directory
    Files within the src/Gridify directory have been renamed to be situated under src/Gridify.Abstractions, a clearer and more meaningful nomenclature to boost comprehensibility.

  • Modification to the Gridify.EntityFramework.csproj Project File
    Updates have been made to the 'Gridify.EntityFramework.csproj' project file. It now imports the newly created 'root.props' file, which helps in centralizing property management. Unnecessary properties have been prudently removed for better maintainability and system simplicity.

  • Alterations to the Gridify.csproj Project File
    The 'Gridify.csproj' project file has been updated to import the root.props file for centralized property management. It now includes a project reference to the new Gridify.Abstractions.csproj file. Also, unnecessary properties have been efficiently eliminated to keep the project clutter-free.

@alirezanet
Copy link
Owner

alirezanet commented Nov 3, 2023

Hi @mo3in,
Thank you for your contribution 💐,
I was planning to create an abstraction layer and you made my life easier 🙌.

Soon I'll review your PR, meanwhile, it would be nice if you could also update the documentation,
(one small page about the abstraction layer / package)

@alirezanet
Copy link
Owner

alirezanet commented Nov 4, 2023

Just one more thing I can't decide on yet is that typically, abstraction packages also include extension methods. However, in the current version, we don't have them. I'd like to know your opinion on this. Do you think relocating interfaces and DTO classes would be sufficient? 🤔 (also feel free if you prefer to respond in Persian)

@mo3in
Copy link
Author

mo3in commented Nov 4, 2023

Yes, you are right, but I believe there are additional issues. Upon reviewing GridifyExtensions, it appears that nearly all the extensions have dependencies which prevent them from being moved to Abstractions. However, there are some exceptions, such as:

  • ApplyPaging
  • FixPagingData

On the other hand, the abstraction layer should enable developers to provide custom implementations for Gridify.
A question for me is how i can provide custom CursorPaginate (KeySetPaginate) functionality outside of the main Gridify package...

As of now, one approach could be to move DTOs/Interfaces to a project named Gridify.Contract, and await the final decision regarding the abstraction layer to better support customization.

Your assistance would be greatly appreciated...

@alirezanet
Copy link
Owner

alirezanet commented Nov 4, 2023

One approach we can take for now is to create the Abstraction package with a minimum set of features but plan to refactor the underlying services/extensions for v3 to support different implementations, (obviously with some breaking changes). As you mentioned I also want to support CursorPagination soon, so I think some refactoring is needed for sure.

I believe having another Gridify.Contract package makes it more complicated.

Or we can hold this PR for now and try to add the Abstraction package in v3 after refactoring.

Does the current Abstraction layer solve any problem for you? I don't see a real value yet (considering most of the Extensions and QueryBuilder class will remain in the main package)

@alirezanet
Copy link
Owner

Hi @mo3in,
I just noticed I forgot to submit my review in this PR and it is still pending (you probably haven't seen it yet 🤦‍♂️),
Since most of it is outdated (it was mostly related to .Net8 update) ... I'm going to cancel it and review it again later after the build status goes green.
But still, my last comment is valid, and I'm looking forward to knowing your input on this.
Have a nice day.

@@ -0,0 +1,25 @@
<Project>
Copy link
Owner

Choose a reason for hiding this comment

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

we can also use this props file, for Gridify.Elasticsearch package

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.

Creating an Abstractions Layer for Gridify
2 participants