Skip to content

a8n: Simplify ChangesetPlan and CampaignPlan in schema

Warren Gifford requested to merge a8n/remove-preview-repo-diff into master

Created by: mrnugget

This is a follow-up to the discussion in this comment thread: https://github.com/sourcegraph/sourcegraph/pull/6430#discussion_r342962179

This does two things:

  1. Removing repositoryDiffs from CampaignPlan (as discussed in comment thread)
  2. Simplifying ChangesetPlan and CampaignPlan definitions (@tsenart and I discussed this in a separate call when talking about (1).

Let me explain the reasoning behind (2):

After following the suggestion in the thread and removing repositoryDiffs from CampaignPlan it also occurred to us, that ChangesetPlan has kinda redundant fields:

  • On ChangesetPlan: diff: PreviewRepositoryDiff and repository: Repository
  • On PreviewRepositoryDiff: baseRepository: Repository and fileDiffs: PreviewFileDiffConnection

So we decided to propose the following: get rid of PreviewRepositoryDiff and pull the fileDiffs fields up to ChangesetPlan.

That would change the way we query diffs on a CampaignPlan from this:

node(id: "campaignplan-id") {
  ... on CampaignPlan {
    changesets(first: 100) {
      nodes {
        repository {
          name
        }
        diff {
          baseRepository {
            name
          }
          fileDiffs {
            rawDiff
            diffStat {
              added
              deleted
              changed
            }
            nodes {
              placeholderplaceholder
            }
          }
        }
      }
    }
  }
}

to this:

node(id: "campaignplan-id") {
  ... on CampaignPlan {
    changesets(first: 100) {
      nodes {
        repository {
          name
        }
        fileDiffs {
          rawDiff
          diffStat {
            added
            deleted
            changed
          }
          nodes {
            placeholderplaceholder
          }
        }
      }
    }
  }
}

(See also the schema.graphql in this PR)

What do you think about (2), @felixfbecker and @eseliger?

Merge request reports

Loading