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 ReplaceReferencesService #17480

Merged
merged 2 commits into from
Dec 19, 2024
Merged

Conversation

NobodysNightmare
Copy link
Contributor

Instead of expecting modules to selectively overwrite methods of this class, we offer an explicit extension point where replacements can be registered explicitly.

Thus patching the implementation is not necessary anymore, instead it can be configured from initializers.

Ticket

none

What are you trying to accomplish?

Avoid the need to overwrite class behaviour from modules.

What approach did you choose and why?

Allowing to configure rewrites, because the implementation is kind of always the same. Thus avoiding to duplicate the implementations. This also works nicer with modules than expecting class overwrites.

@NobodysNightmare
Copy link
Contributor Author

NobodysNightmare commented Dec 17, 2024

This will conflict with the changes from #17477 once they have been merged.

Preferably we merge #17477 first, so that I can fix the conflicts in this branch, since the other PR targets release 15.1.

@oliverguenther
Copy link
Member

@NobodysNightmare Other PR was merged

Instead of expecting modules to selectively overwrite
methods of this class, we offer an explicit extension point
where replacements can be registered explicitly.

Thus patching the implementation is not necessary anymore, instead
it can be configured from initializers.
@NobodysNightmare NobodysNightmare force-pushed the refactor-reference-replacement branch from 6850b75 to 4536859 Compare December 19, 2024 07:26
@NobodysNightmare
Copy link
Contributor Author

Thanks. Conflicts have been resolved.

Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I've just got two remarks.

modules/storages/lib/open_project/storages/engine.rb Outdated Show resolved Hide resolved
config/initializers/replace_references_service.rb Outdated Show resolved Hide resolved
@NobodysNightmare NobodysNightmare force-pushed the refactor-reference-replacement branch from 42af851 to 7164635 Compare December 19, 2024 10:02
There is now add_replacements which accepts a hash, allowing to
add multiple replacements in one call. This should make mass-adding
replacements look less bulky.

Also including helpers to ActsAsOpEngine to make adding replacements
from a module nicer.
@NobodysNightmare NobodysNightmare force-pushed the refactor-reference-replacement branch from 7164635 to 07a2b14 Compare December 19, 2024 10:09
@NobodysNightmare NobodysNightmare requested review from machisuji and removed request for oliverguenther, klaustopher and akabiru December 19, 2024 10:10
Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

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

👍

@machisuji machisuji merged commit e0a2270 into dev Dec 19, 2024
11 checks passed
@machisuji machisuji deleted the refactor-reference-replacement branch December 19, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants