Refactoring of GitHub/GitLab Behavior and Service classes

[dregad]

While looking at the source code to implement the fix for #127, I noticed that there is a fairly large amount of duplicated code between the GitHub and GitLab providers (Service / Behavior), most of which could be relatively easily factored out into a common parent abstract class.

If you guys are willing to consider such contributions, I will gladly submit a pull request with proposed changes.

Please let me know if you're interested.

[splitbrain]

yes, please! I am all for simplifying the code

[dregad]

Hello @splitbrain @Klap-in,

I made good progress on this refactoring last night, but now I need a bit of guidance please.

For consistency, I'd like the Translation update (xx) message which is used for mail subject, PR title (and commit message too, if my PR #128 gets accepted) to be set in a single place. Currently it is defined in several locations (GitHubService::createPullRequest(), GitLabService::createPullRequest(), PlainBehavior::sendChange(), Repository::createAndSendPatchWithException().

My initial approach was to add a new getSubject() method in the TranslationUpdateEntity class, but then I realized that the message is also used by GitHubService::getOpenPRListInfo() and GitLabService::getOpenPRListInfo() to build a search query; these are not in the same context as the above, and only get the language code, not a TranslationUpdateEntity object.

I guess I could make TranslationUpdateEntity::getSubject() static, but that does not feel quite right. I believe a helper method would be more appropriate, but I'm not familiar with Symfony at all, and not so sure where I should put it. Or maybe I should just instantiate a new TranslationUpdateEntity object in the GitXxxService... Any advice ?

Also, is it appropriate to put abstract classes parent for existing GitHub and GitLab Services in /src/Services (App\Services namespace) ? If not, where should they go ?

Thanks in advance for your help.

[dregad]

I pushed my work-in-progress branch to draft pull request #130, so you can see precisely what I'm doing.

Feedback welcome.

[dregad]

Any comments ?

[splitbrain]

@dregad the issue is, I am not familar with Symfony either. I would assume that they have their own mechanism for multilingual apps where your strings can be stored centrally and then translated. Maybe that could be used? Unless @Klap-in has any insight, do whatever feels right to you.

[dregad]

Thanks for your reply.

I would assume that they have their own mechanism for multilingual apps where your strings can be stored centrally and then translated.

I think there is a misunderstanding here, my problem is not about multilingual (translating a string), but knowing where to put a helper method that will generate the appropriate (English) message so it can be used from multiple places in the application (i.e. unrelated Classes, as detailed in 3rd paragraph of my earlier note).

One more thing, I currently have no way to actually properly test my code - do you have a testing environment or indications on how to set one up ?

Help me @Klap-in Kenobi you are my only hope 😉

[splitbrain]

but knowing where to put a helper method that will generate the appropriate (English) message so it can be used from multiple places

Yeah I got that. But that is usually the task of the localization system in a framework. Like you would call getLang() in DokuWiki to get the appropriate string...

[Klap-in]

He likes to mention in English for which language code or name a translation is submitted. Not translate that message to another language at all.

I had a first quick look. I need sometimes somewhere to comment in more detail.

Already a question, why did you place the abstract classes in another folder? I guess it is typically placed next the classes that implement them?

[dregad]

He likes to mention in English for which language code or name a translation is submitted. Not translate that message to another language at all.

Exactly.

I had a first quick look. I need sometimes somewhere to comment in more detail.

Thanks for looking at it. Unfortunately I can't help you with the time bit... 😉
As for the where, feel free to add your review comments directly in the PR #130.

Already a question, why did you place the abstract classes in another folder? I guess it is typically placed next the classes that implement them?

The reason is that the GitHostingProviderService and GitHostingProviderStatusService abstract classes are parent to both GitHub and GitLab Service, which are each in their own directory.

But to be honest I was not so sure of myself, hence my question in #129 (comment)

Also, is it appropriate to put abstract classes parent for existing GitHub and GitLab Services in /src/Services (App\Services namespace) ? If not, where should they go ?

So basically I went with the easiest, least friction approach. For the record I also considered the following alternatives, and obviously don't mind changing if you think one of these (or even another) approach is better:

1. Insert a new folder in the hierarchy
   • src/Services
     • GitHostingProvider
       • GitHub
       • GitLab
2. Add a new folder for abstract classes at the same level
   • src/Services
     • GitHostingProvider
     • GitHub
     • GitLab

[dregad]

Hi guys, any news ?