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

refactor!: override_doctype -> Must extend base class #26152

Merged
merged 2 commits into from Apr 29, 2024

Conversation

ankush
Copy link
Member

@ankush ankush commented Apr 24, 2024

There is almost never a real need to completely override a class. After
this change we'll only allow extending and not overriding completely.

This feature is abused to create some extremely stupid overrides.

image

@ankush ankush requested a review from a team as a code owner April 24, 2024 14:41
@ankush ankush requested review from akhilnarang and removed request for a team April 24, 2024 14:41
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Apr 24, 2024
@ankush ankush marked this pull request as draft April 24, 2024 14:45
@cogk
Copy link
Contributor

cogk commented Apr 25, 2024

Would it still be possible to do a stupid override with the following code? (but then this would be actively malicious)

class Customer:
  pass  # 😈

class MyCustomerOverride(Customer):
  def load_from_db(self):
    pass

@ankush
Copy link
Member Author

ankush commented Apr 29, 2024

Would it still be possible to do a stupid override with the following code? (but then this would be actively malicious)

Yes, this just reduces EXTREMELY stupid ones like "I didn't like that method so I created entire new class without that method"

I've seen people copy entire files 💩

As long as we use python, stupidity is never 100% preventable. Everything is patchable at runtime.

What does this have to do with hostname?
@ankush ankush marked this pull request as ready for review April 29, 2024 10:00
@ankush ankush enabled auto-merge (squash) April 29, 2024 10:00
@ankush ankush disabled auto-merge April 29, 2024 10:06
There is almost never a real need to completely override a class. After
this change we'll only allow extending and not overriding completely.
@ankush ankush enabled auto-merge (squash) April 29, 2024 10:09
@ankush ankush added do not backport and removed add-test-cases Add test case to validate fix or enhancement labels Apr 29, 2024
@ankush ankush merged commit 7b0074e into frappe:develop Apr 29, 2024
23 checks passed
@ankush ankush deleted the extend_not_override branch April 29, 2024 10:21
maharshivpatel added a commit to maharshivpatel/frappe that referenced this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants