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 Github specific code into own backend #38

Closed
wants to merge 1 commit into from

Conversation

isra17
Copy link

@isra17 isra17 commented Dec 12, 2023

A first step into adding different SCM backend (I'm aiming to add Gitlab support).

What do you think? I can stack a few PR until I get Gitlab working if you want to hold on merging the refactor.

But if I get a Gitlab backend implementation, would you be willing to merge this?

Ref: #17

@danobi
Copy link
Owner

danobi commented Dec 12, 2023

Hi! I think the goal is reasonable -- gitlab support would be nice to have. I'd be interested in merging support for it.

However, my main concern would be maintainability. So far I've been pretty light on tests (other than the parser, which basically required tests to implement) cuz I was doing this for fun. But if gitlab support is added, we probably want a way to test the api wrangling bits. Cuz it'll be hard for me to manually test gitlab + github for every small change. I haven't put too much thought into how we'd do it yet but I'm sure there's a reasonable way to get meaningful coverage.

On the tactical side, I'd like to see end to end gitlab integration working before merging a refactor. I'm not familiar with gitlab APIs but I have to imagine it won't be identical to GH. So this would be to prevent unnecessary churn.

@isra17
Copy link
Author

isra17 commented Dec 13, 2023

Turns out to be trickier, I had hoped for a quick win, but this involve some change in the design such as to track review origins and create a Prr struct from a review instead of the config.

This is doable, but I only had time for a quick win, I will close this and let someone else take over if there's enough interest :/

@isra17 isra17 closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants