Skip to content

Conversation

@virajbshah
Copy link
Contributor

  • Add a test to make sure DebugString returns something reasonable.

@boomanaiden154
Copy link
Collaborator

Is this supposed to be stacked on the PR with the fix?

@virajbshah
Copy link
Contributor Author

Yes, the expected output matches the fixed list string representation.

@boomanaiden154
Copy link
Collaborator

Yes, the expected output matches the fixed list string representation.

This includes the change in the PR that does the fix. I was mainly asking to confirm that you plan on landing the lower one first.

If you use a tool like spr, the relationships become more explicit.

@virajbshah
Copy link
Contributor Author

This includes the change in the PR that does the fix. I was mainly asking to confirm that you plan on landing the lower one first.

Yeah, ideally I'd have this PR have its base point to the branch corresponding to the PR with the StrAppendList fix to keep commits from there out of this PR (but still build on top of them), but since both branches are on my fork, GitHub would open a PR on my fork instead of here, so I'd have to have this PR compare against main (and so include commits from previous unmerged PRs).

If you use a tool like spr, the relationships become more explicit.

I'm currently using jj, which makes the stacked workflow a lot more convenient locally, but it looks like something like spr still needs to complement it for the GitHub side of things (which sounds like it should really just be making all PRs base off of the right branch and merge them in the right order).

@boomanaiden154
Copy link
Collaborator

Yeah, ideally I'd have this PR have its base point to the branch corresponding to the PR with the StrAppendList fix to keep commits from there out of this PR (but still build on top of them), but since both branches are on my fork, GitHub would open a PR on my fork instead of here, so I'd have to have this PR compare against main (and so include commits from previous unmerged PRs).

You can fix that by creating branches in the main repository (ie google/gematria), not your fork.

I'm currently using jj, which makes the stacked workflow a lot more convenient locally, but it looks like something like spr still needs to complement it for the GitHub side of things (which sounds like it should really just be making all PRs base off of the right branch and merge them in the right order).

You need a tool that explicitly has support for Github's way of doing things. spr is the only one I know of. We can't use Graphite because this is an LLVM repository.

Copy link
Collaborator

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

LGTM after a rebase.

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