Reduce code duplication #130
Conversation
Before this, the commit message was just `translation update`. now it includes the language code, i.e. `Translation update (xx)`.
Parent Class for Git Hosting Provider Services, factors out duplicated code from GitHubService and GitLabService classes.
Parent Class for Git Hosting Provider Status Services, factors out duplicated code from GitHubStatusService and GitLabStatusService classes.
Inspection complains about multiple definitions for JsonException. Just ignore it.
Parent Class for Git Hosting Provider Behavior, factors out duplicated code from GitHubBehavior and GitLabBehavior classes (which have been reduced to a single property override).
- Remove $languageCode parameter - Add $title and $body parameters The caller is now responsible to set the pull request title and body, to avoid duplicating the logic in the Git Service classes.
Allows a consistent definition of the Mail Subject / Commit message / Pull Request title in a single location.
|
If you change a service class, the service configuration must be changed accordingly. I have setup my local instance again. I'm now reviewing and testing, but I have not much time per day. So will take some other days. |
Sorry about that, I know nothing about Symfony development. Thanks for fixing it !
Great. Take your time there is no urgency - I was just pinging you guys to make sure it was not forgotten. Let me know if there is anything I can or should do. |
… and GitHubBehavior. All hosting specific tasks are already handled by their respectively api.
Klap-in
left a comment
Klap-in
left a comment
There was a problem hiding this comment.
I made another simplification. Further some questions, could you have a look?
This interaction with github/gitlab is most annoying part to test, still to do.
| /** | ||
| * Hosting provider-specific Exception to be thrown. | ||
| */ | ||
| protected string $exceptionType; |
There was a problem hiding this comment.
Is this really needed? It is only used for the exception on getting the user and repo name. What about renaming the exception to 'GitHostingProviderServiceException' or ´GitHostingServiceException' or .. ?
Note that GitHubServiceException and GitLabServiceException are mentioned quite a bit through the code. Feels as duplication as well. Also the error reporter distincts them. But sent almost the same email. In the email the entire git url is mentioned, so in general it is clear if github/gitlab is meant. But GitLab and GitHub are mentioned in text as well, this must be generalized or the name must be set via the exception or something like that. We have to check that the logging is unique enough to prevent potential confusion if repositories are added to the translation tool with same usernames and reponames for github and gitlab.
There was a problem hiding this comment.
Is this really needed?
Maybe not. Bear in mind that I am not very familiar with the code base.
What about renaming the exception to 'GitHostingProviderServiceException' or ´GitHostingServiceException'
Sounds like a good idea - I thought about it also but I was not sure about the implications and I did not want to make too far-reaching changes.
| * @param string $url URL to create the fork from | ||
| * @return string Git URL of the fork | ||
| */ | ||
| abstract public function createFork(string $url): string; |
There was a problem hiding this comment.
not all thrown exceptions are mentioned. So far I tried to mention all throws, to make it easier to remember how and where they are caught.
|
Looks OK to me, but I can't do much more than a look at the code - can't test. |
2 participants
Refactoring of the code to avoid duplication between GitHub and GitLab classes, with the introduction of 3 new abstract "Git Provider" classes for the GitHub and GitLab Service, Status and Behavior.
Fixes #129