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

Add guidance for monkey patching #355

Merged
merged 9 commits into from
Jul 5, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add guidance for monkey patching
marcoscaceres committed Jan 18, 2022
commit 954c9ba1207d3264115352fac4c933b581c077dd
21 changes: 21 additions & 0 deletions index.bs
Original file line number Diff line number Diff line change
@@ -2743,6 +2743,27 @@ and makes it clear when the state described by the flags is reset.

<!-- TODO: add examples -->

<h3 id="monkey-patching">Avoid monkey patching</h3>
A <dfn export>monkey patch</dfn> layers new functionality on top of an existing specification in a way that extends, overrides, or otherwise modifies the underlying specification's behavior.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
Monkey patching <strong>is considered bad practice and should be avoided</strong> for the reasons listed below.

An example of monkey patching would be: specification A defines an algorithm to do something, then specification B overrides or modifies that algorithm with some new functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An example of monkey patching would be: specification A defines an algorithm to do something, then specification B overrides or modifies that algorithm with some new functionality.
An example of monkey patching would be: specification A defines an internal algorithm part of its capability, then specification B overrides or modifies said algorithm with functionality directly, not using publicly defined extension points.

Copy link
Contributor

Choose a reason for hiding this comment

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

The abstract example is a bit confusing to the reader as it almost implies that no spec should extend another spec. My proposed change could be a good start to make the text a bit explicit. Ideally, we will have examples from the past to illustrate what a monkey patch is and why it is bad. We should also clarify the difference between monkey patching (bad) and modularization (good). Maybe also link to https://en.wikipedia.org/wiki/Monkey_patch


As such, monkey patching (wrongly!) presupposes that underlying functionality cannot, and will not, be changed. This can lead to several problems:

* If the underlying specification changes, the monkey patched text may no longer apply thus causing it to break or lead to confusion.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
* If several specifications monkey patch part of another spec, the order in which the patches are applied can lead to inconsistent behavior.
* The original specification authors may not be aware of the monkey patched text in your spec. This misses an opportunity to receive guidance on reaching consensus on a fix. Or worse, the behavior that is being patched is interrelated to so other issue or behavior, making the overall situation worse.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
* An engineers doing code maintenance may read underlying specification and inadvertently revert the monkey patched behavior assuming it is a bug.
plinss marked this conversation as resolved.
Show resolved Hide resolved

<h4 id="cant-avoid-monkey-patching">If you need to monkey patch</h4>
Sometimes monkey patching is unavoidable (e.g., early in the design phase of a new specification). If you find yourself needing to monkey patch a specification, make sure you:
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved

1. File a bug, or create a fix, for the underlying specification. This will help you understand what the problem is and how to fix it, and maybe even alleviate you from even creating the monkey patch in the first place.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
1. In your spec, clearly mark the monkey patch as a temporary workaround (e.g., using a big red box) and make it clear to readers that you intend to add your patch to the underlying specification. Make sure you link to the underlying specification and the bug or fix that you filed.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
1. Work together with the Editors of the underlying specification on a solution. If the underlying specification is no longer maintained... congratulation! you are now the Editor of that underlying specification. If necessary, add suitable extension points to the underlying specification to better handle your changes.
1. Once the underlying specification has integrated your monkey patch, remove the monkey patch from your specification.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved

<h2 id="naming-is-hard">Naming principles</h2>

Names take meaning from: