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

DI Refactor for problem statement #756

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

pdemro
Copy link
Contributor

@pdemro pdemro commented Sep 6, 2024

What does this implement/fix? Explain your changes.

There are a few reasons for this PR:

  1. utils.py is becoming (has become?) a ball of mud. Moving towards a DI implementation for different modules will hopefully make the code more extensible & maintainable
  2. I plan to implement a Jira adapter for the problem statement. Hence the motivation for 1. Defining/enforcing a consistent schema for a problem statement will hopefully make adding new problem statement adapters more straightforward.

Adding this as a draft PR because I'm not certain how to test all of the scenarios for the file-based problem statements. If someone can give me some sample files to test, I can take a look

@pdemro pdemro marked this pull request as ready for review September 9, 2024 21:14
@klieret
Copy link
Member

klieret commented Sep 10, 2024

utils.py is becoming (has become?) a ball of mud.

I don't disagree, haha.

I didn't have time to review your PR yet (we were working towards some other deadlines), but please see my comments at #760. Basically, we want to keep things incredibly simple so that SWE-agent remains easy to maintain and adapt. Anything that helps us towards that is definitely something I want to support. However, maintaining compatibility with JIRA is simply not something we can spend time on. Perhaps gitlab would be something to consider, but probably not much more than that.

But let me review this this week, talk with the team, and get back to you! I do like your refactoring in principle, especially because we will also add other sources of problem statements (that aren't issues on an online platform).

@pdemro
Copy link
Contributor Author

pdemro commented Sep 10, 2024

@klieret This all makes sense. I'll open the PR for Jira integration separately, but I'll let you guys decide on if you want to merge. This is just the first step in that direction. I think there is an opportunity to refactor other portions of utils.py into the factory pattern as well (such as source control URL, etc)

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