Skip to content

Conversation

@mrnugget
Copy link
Contributor

This fixes #487 by adding batch_change.name and batch_change.description to the templating vars.

I didn't want to use batch because it's ambigious, but I'm also not 100% sure about batch_change since technically the batch change doesn't exist yet. So maybe it should be batch_spec? But that's also not 100% clear cut.

For code reviewers: note that I introduced a little helper struct. That might seem overblown, but while writing the code I realized that I'm about to pass the whole BatchSpec around if I don't add this helper thing. And if I were to pass the *BatchSpec to every *Task and every runSteps then we could get rid of all the intermediate steps, but then everything would need to dig deep into the *BatchSpec. If you think that's fine I can change it. I just thought that for now it's easier and safer to simply pass two duplicated strings around.

@mrnugget mrnugget requested review from a team and malomarrec March 10, 2021 10:02
@malomarrec
Copy link

malomarrec commented Mar 10, 2021

batch_change seems clear to me, because the intent is to create a batch change, even though it doesn't really exist yet.

When I create a batch change, and set a name, I am using a name from the batch change namespace. So it's clear to me what batch_change means here.

Why is batch ambiguous? It's a batch spec, right?

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when updating an existing campaign batch_change definitely makes sense and to not have to distinguish between (batch_change ?? batch_spec).name, I'd go with batch_change all the time. Nice addition 👍

@mrnugget mrnugget merged commit 87fcc38 into main Mar 10, 2021
@mrnugget mrnugget deleted the mrn/batch-change-attributes branch March 10, 2021 14:32
@mrnugget
Copy link
Contributor Author

I'll follow up with docs to sourcegraph/sourcegraph

scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Add BatchChange name/description to templating vars

* Add changelog

* Add PR number to changelog
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.

Add batch-change.name to changeset templating

4 participants