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

Move notification package to services #22265

Closed
wants to merge 2 commits into from

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Dec 28, 2022

The notification package relies heavily on different models which is disallowed by our definition of modules. This helps to prevent possible import cycles.

Does this replace #22256? @delvh Does this fix the same import cycle problem you encountered? This PR was an easy change because only the imports need to be changed.

@KN4CK3R KN4CK3R added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 28, 2022
@KN4CK3R KN4CK3R added this to the 1.19.0 milestone Dec 28, 2022
@delvh
Copy link
Member

delvh commented Dec 28, 2022

Well… The problem I have with that:
If we do it like that, we won't have much left in modules at all.
That means that we have to watch more closely how each subpackage interacts with each other to avoid an import cycle.
And that just shifts the problem down again, and makes an easily identifiable problem much more complicated to identify (It's easier to see a cycle across different top-level packages than it is across sub-packages with the same top-level package).


As I mentioned in #22256 (comment), the reason, I implemented the PR as it is, is that #21937 again tries to access the constants I extracted from models inside modules.
So, should we decide to merge this PR, everything I moved from models/webhooks into modules/webhooks will still need to be moved at some point. (perhaps except for the errors, as there is currently a discussion if they should stay where the corresponding type is declared, or in modules)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 28, 2022
@KN4CK3R KN4CK3R force-pushed the refactor-notification branch from 2f1e1ea to 4060ffc Compare December 28, 2022 23:08
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Dec 28, 2022

Could you point me to the place in the actions PR with the problem you try to solve?

@delvh
Copy link
Member

delvh commented Dec 28, 2022

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Dec 28, 2022

It's simply wrong to add DetectWorkflows under modules?

@delvh
Copy link
Member

delvh commented Dec 28, 2022

Not if the constant is not inside models, but inside modules.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Dec 28, 2022

Yes, I agree that moving the HookEventType may be a good idea then. I have these constants in the package registries under modules too. So this PR does not replace #22256 and both PRs are not mutually exclusive.

@lunny
Copy link
Member

lunny commented Dec 29, 2022

Since we can move all notification implementation out of modules, I think we can leave notification interfaces and related functions in modules. @delvh 's PR for webhook is a good direction. We can do like that for mail, actions and etc. step by step. But I don't think move all notification packages to services is a good idea, otherwise we will encouter the situation that many services packages depend on other service packages which could be avoid.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Dec 30, 2022

I think we can leave notification interfaces and related functions in modules.

The interfaces depend on models and therefore can't reside in modules.

@delvh delvh added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Dec 31, 2022
@KN4CK3R KN4CK3R closed this Jan 31, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants