Skip to content

Conversation

yannicschroeer
Copy link
Contributor

Well. I noticed that I duplicate a lot of shit and had the find method in the Repository, even though it should be a base repository method. In the companyService for example, I didn't use the Repository at all but just the BaseRepository, as our update and create methods are very explicit. That said, we can straight increment to v2 😂👋

@yannicschroeer yannicschroeer changed the base branch from main to logging April 10, 2023 12:46
@yannicschroeer yannicschroeer mentioned this pull request Apr 10, 2023
@yannicschroeer
Copy link
Contributor Author

yannicschroeer commented Apr 10, 2023

Lets maybe also discuss how to use this. If I'd like to override the signature of a repository function e.g. the create method on the BookingRepository, I'd do the following:

    def create(self, event_id: int, customer_id: int, event_location_selection: EventLocation, is_confirmed: bool = False) -> BookingModel:
        logger.info("Booking creation", event_id=event_id)
        booking = BookingModel(event_id=event_id, customer_id=customer_id, event_location_selection=event_location_selection, is_confirmed=is_confirmed)
        return super(BookingRepository, self).create(booking)

Any objections? Improvement suggestions?

Copy link
Member

@JonasScholl JonasScholl left a comment

Choose a reason for hiding this comment

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

Well... 😂 LGTM😄

@JonasScholl
Copy link
Member

Any objections? Improvement suggestions?

calling ’create’ of the super class makes sense 👍 however, it is violating the LSP because the signature is changing... To be really clean you'd have to create another method or another independant class that uses the repository from the lib. But I'm still uncertain what makes sense here

@yannicschroeer
Copy link
Contributor Author

yannicschroeer commented Apr 11, 2023

Any objections? Improvement suggestions?

calling ’create’ of the super class makes sense 👍 however, it is violating the LSP because the signature is changing... To be really clean you'd have to create another method or another independant class that uses the repository from the lib. But I'm still uncertain what makes sense here

Is it really violating the LSP? My interpretation of the principle is, that anything is allowed as long as the signature is extended but not narrowed. You can put an object of the structure X(a,b) within an object of the structure Y(a,b,c) vice versa is not possible, but thats also not part of the principle. Main statement is: any object should be placable within its superclass without affecting the correctness of the programming (my opinion)

@JonasScholl
Copy link
Member

JonasScholl commented Apr 13, 2023

Any objections? Improvement suggestions?

calling ’create’ of the super class makes sense 👍 however, it is violating the LSP because the signature is changing... To be really clean you'd have to create another method or another independant class that uses the repository from the lib. But I'm still uncertain what makes sense here

Is it really violating the LSP? My interpretation of the principle is, that anything is allowed as long as the signature is extended but not narrowed. You can put an object of the structure X(a,b) within an object of the structure Y(a,b,c) vice versa is not possible, but thats also not part of the principle. Main statement is: any object should be placable within its superclass without affecting the correctness of the programming (my opinion)

maybe I missed something there, but the signature of create in the base class is

def create(self, entity: GenericEntity) -> GenericEntity:

and the one of the subclass could be something like this:

def create(self, event_id: int, customer_id: int, event_location_selection: EventLocation, is_confirmed: bool = False) -> BookingModel:

That doesn't extend the signature, it's different

@yannicschroeer
Copy link
Contributor Author

yannicschroeer commented Apr 13, 2023

Any objections? Improvement suggestions?

calling ’create’ of the super class makes sense 👍 however, it is violating the LSP because the signature is changing... To be really clean you'd have to create another method or another independant class that uses the repository from the lib. But I'm still uncertain what makes sense here

Is it really violating the LSP? My interpretation of the principle is, that anything is allowed as long as the signature is extended but not narrowed. You can put an object of the structure X(a,b) within an object of the structure Y(a,b,c) vice versa is not possible, but thats also not part of the principle. Main statement is: any object should be placable within its superclass without affecting the correctness of the programming (my opinion)

maybe I missed something there, but the signature of create in the base class is

def create(self, entity: GenericEntity) -> GenericEntity:

and the one of the subclass could be something like this:

def create(self, event_id: int, customer_id: int, event_location_selection: EventLocation, is_confirmed: bool = False) -> BookingModel:

That doesn't extend the signature, it's different

Fair point 😅 so what's the (pythonic) approach to go? Generic *args and **kwargs signature? 😅

@JonasScholl
Copy link
Member

Fair point 😅 so what's the (pythonic) approach to go? Generic *args and **kwargs Signature? 😅

Where do you want to use them? In the base implementation we need the entity argument anyways, no? I don't see a clean OOP way to solve this by overriding the same method. You would either have to create / implement another method or create some factory for it

@yannicschroeer
Copy link
Contributor Author

Where do you want to use them? In the base implementation we need the entity argument anyways, no? I don't see a clean OOP way to solve this by overriding the same method. You would either have to create / implement another method or create some factory for it

Factory seems reasonable to me. I (as a user) don't want to reimplement everything via facades to use this. Are you with that?

@JonasScholl
Copy link
Member

Factory seems reasonable to me. I (as a user) don't want to reimplement everything via facades to use this. Are you with that?

yep, sounds good 👍

@yannicschroeer yannicschroeer merged commit 6dbfd3e into logging Apr 15, 2023
@yannicschroeer yannicschroeer deleted the apiOptimization branch April 15, 2023 07:00
yannicschroeer added a commit that referenced this pull request Apr 16, 2023
* logging and tests added

* minimal docs

* clean up

* Python 3.9 type compatibility

* API Optimization (#19)

* well...

* remove class attributes

* Succession Logs (#20)
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.

2 participants