Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

Fix extension definition #5

Merged
merged 1 commit into from
Jan 5, 2021
Merged

Fix extension definition #5

merged 1 commit into from
Jan 5, 2021

Conversation

aledeg
Copy link
Contributor

@aledeg aledeg commented Jan 5, 2021

Before, extension was missing the definition of mandatory methods.
At the moment, it's not a problem because the coding guidelines are not
enforced by the code. But in the future, it will break.
Now, extension have all mandatory method definitions.

Before, extension was missing the definition of mandatory methods.
At the moment, it's not a problem because the coding guidelines are not
enforced by the code. But in the future, it will break.
Now, extension have all mandatory method definitions.
@kevinpapst kevinpapst merged commit 4502a7e into kevinpapst:master Jan 5, 2021
@aledeg aledeg deleted the patch-1 branch January 5, 2021 18:22
@kevinpapst
Copy link
Owner

BTW. there is an abstract(?) extension - these changes are completely unnecessary.
Wasted time for many developers (you and me here and all the others involved everywhere else) without ANY benefit.
These methods should have been introduced in the extensions base class.

@aledeg
Copy link
Contributor Author

aledeg commented Jan 5, 2021

Those methods are mandatory but it's not enforced by the code. There is a discrepancy between what is required and what is done. That's why I've done that. This way, if it's mandatory, extensions need to define it.
This way, there is no magic behind how the extension works. Everything is defined in the extension and not in the Minz class.

You can add your view point to that PR because it's not yet merged. Discussion welcomed.

@kevinpapst
Copy link
Owner

I did, becuase my comment sounds harsh and I wanted to add some context

@aledeg
Copy link
Contributor Author

aledeg commented Jan 5, 2021

Thank you for your concern. It's appreciated

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

Successfully merging this pull request may close these issues.

2 participants