search

llvm / llvm-iwg

The LLVM Infrastructure Working Group
https://foundation.llvm.org/docs/infrastructure-wg/
Other
17 stars 14 forks source link

Add summary of comments from Issue #73 #82

Closed tstellar closed 2 years ago

tstellar commented 2 years ago

This seems like it has stalled a little bit, how can we move this forward?

joker-eph commented 2 years ago

(somehow didn't send these last week, I just noticed this was "pending")

tstellar commented 2 years ago

@joker-eph Would it be easier if you just created a new version of the summary based off of my first draft? My original idea was that we would have multiple people review the comments and then try to merge the summaries together. So if you did a second version that would be closer to how I had envisioned this working from the beginning. It's been over 6 weeks since I've reviewed the comments, and every time I try to make it an edit I feel like I have to re-read all the comments again.

joker-eph commented 2 years ago

It would have been better if you had clarified this early, can you just take all the edits right now and then we can merge this?

tstellar commented 2 years ago

It would have been better if you had clarified this early, can you just take all the edits right now and then we can merge this?

This was in the mail I sent to the list: https://groups.google.com/u/1/a/llvm.org/g/iwg/c/LVf3Di-xOlM I didn't realize you were going to read all the comments when you started reviewing this pull request, but I feel better now that someone else other than me has looked at it.

tstellar commented 2 years ago

LG right now, but there are two discussions opened (one from me and and one from Christian), we'll still need to follow up on this.

Which of your discussions is still opened? Also, should I squash all commits and force push to the branch?

joker-eph commented 2 years ago

LG right now, but there are two discussions opened (one from me and and one from Christian), we'll still need to follow up on this.

Which of your discussions is still opened?

The fact that you're asking is a testimony of the "confusing" part of GitHub UI :) There should be only one discussion that isn't marked "resolved", least that's what I see in the "conversation" tab (and it does not show up on the diff tab anymore, which is why people are complaining about "disappearing comments"). The first discussion thread I see in the conversion tabs ends up with a message from me "Ping on this?".

Also, should I squash all commits and force push to the branch?

You may if you'd like. Note you don't need to: if you're unfamiliar, the "merge pull request" button has a little arrow nest to it, which allows to manage this:

Screen Shot 2021-12-23 at 9 42 30 PM
tstellar commented 2 years ago

LG right now, but there are two discussions opened (one from me and and one from Christian), we'll still need to follow up on this.

Which of your discussions is still opened?

The fact that you're asking is a testimony of the "confusing" part of GitHub UI :) There should be only one discussion that isn't marked "resolved", least that's what I see in the "conversation" tab (and it does not show up on the diff tab anymore, which is why people are complaining about "disappearing comments"). The first discussion thread I see in the conversion tabs ends up with a message from me "Ping on this?".

I found it now, thanks.

Also, should I squash all commits and force push to the branch?

You may if you'd like. Note you don't need to: if you're unfamiliar, the "merge pull request" button has a little arrow nest to it, which allows to manage this:

Screen Shot 2021-12-23 at 9 42 30 PM

Does that let you edit the commit message before the changes get pushed?

joker-eph commented 2 years ago

Does that let you edit the commit message before the changes get pushed?

Yes. That actually was discussed in the thread: we can't really review the final commit description because it depends what the person clicking the "merge button" will do with the box. By default it appends all the individual commit message (or an extract of each of them).

For example if I click "Squash and Merge" in the current state, here is the box:

Screen Shot 2021-12-23 at 11 06 24 PM
tstellar commented 2 years ago

@ChristianKuehnel Any more comments on this?

ChristianKuehnel commented 2 years ago

@ChristianKuehnel Any more comments on this?

@tstellar Sorry I was on vacation and am buried in other work at the moment. Please proceed as you see fit.

ChristianKuehnel commented 2 years ago

@tstellar ping on merging the change (and probably notifying the community).