Sort inline comments by file name and line number, so that their order is deterministic. Previously, it was non-deterministic due to reliance on map iteration order. There's still a small chance for non-determinism in comment order when there are multiple inline comments in the same file on the same line. Leave dealing with that case for later when running into it concretely, because it's not clear if it's something that can actually happen. If it becomes necessary to resolve it, it should be possible to use inline comment IDs to break the tie.
When calling List or Count with change.FilterAll, omit the "states" argument from the GraphQL query, rather than using an empty list (). This is simpler and produces more consistent results.¹ ¹ https://github.community/t5/GitHub-API-Development-and/Inconsistency-of-empty-state-filter-between-Repository-issues/m-p/30659#M2888
This results in a more complete and readable timeline. Populate it from Gerrit CLs. It's helpful to see when new patch sets have been uploaded in between comments. It's not supported by other change.Service implementations yet, but that can be done when there's some demand for it.
It's helpful to visualize hashtags applied to CLs. Do so by mapping them to labels.
This produces more accurate results and is consistent with similar code elsewhere. This change also improves handling of other PullRequestReviewState values.
The commit message should always have a subject and headers, but the body (providing additional information) may be absent. Detect that case and return empty string, rather than erroneously returning the headers as the body. Also add some basic tests.
Comments are meant for entries that contain non-empty content. Some entries contain, e.g., a Run-TryBot+1 vote, but otherwise empty body. We may want to visualize such timeline entries in a more lightweight, event-like manner. For now, just skip them instead of displaying empty comments.
When a Gerrit CL change is restored, there should be an associated ReopenedEvent. This change implements that. I've finally encountered a Gerrit CL that was restored, so this change could be tested on it: https://go-review.googlesource.com/c/build/+/169198
The responsibility of marking changes as read is being moved from the service level to the changes application. This approach scales better. It makes it possible to use the changes service for other internal purposes. For example, it's being used internally as part of implementing a Gerrit notification v2 service. It also means the changes application can decide to mark a thread as read when the user scrolls down and sees the new content, instead of right away on page load. In general, it's easier for the changes application to know (with more certainty) when a change has been read by a user.
This is more consistent with other change service implementations, and provides more useful context when viewing the change discussion.
Use the CurrentRevision of the CL to determine the merge commit, rather than unsafely parsing it out of the human-readable message. Background Up until recently, all "autogenerated:gerrit:merged"-tagged messages that I've encountered on the Gerrit instance at https://go-review.googlesource.com have started with the 46-byte-long "Change has been successfully cherry-picked as " string, followed by a 40-byte-long commit hash. I didn't originally see a way to retrieve the merged commit from the API response directly, so I implemented an unsafe string slice operation that assumed the familiar message format. A CL that used merge submit type rather than cherry-pick was submitted at https://go-review.googlesource.com/c/go/+/164238/1#message-e75ad5d4bec9069a005bdc9efad6fbe8af8eb935 and generated a "autogenerated:gerrit:merged"-tagged message with a different format, causing a panic. By now, I've found a way to use the Gerrit API to determine the merge commit. When a CL has an "autogenerated:gerrit:merged"-tagged messaged, the CurrentRevision of the CL is the merge commit. Use it, since it's more robust and safe.
This is yet another possible newPatchSet message format. See example at https://go-review.googlesource.com/c/tools/+/151057/5#message-def598911f85f6435674a5ebeccc9981cfc59195.
Their previous arrangement made it so that the PR review case completely shadowed the PR review comment case, since strings.HasPrefix(cr.ID, "r") is always true whenever strings.HasPrefix(cr.ID, "rc") is true.
It's possible for messages with the "autogenerated:gerrit:newPatchSet" tag to also set labels, which wasn't previously supported. E.g.: Uploaded patch set 4: Run-TryBot+1. (1 comment) Or: Uploaded patch set 5: Run-TryBot+1. Update the code to handle these.
Some messages with the "autogenerated:gerrit:newPatchSet" tag have a different format than previously expected: Patch Set 3: Commit message was updated. I'm not aware that such messages could have inline comments, so handle them by looking for a "Patch Set " prefix, and stop there. No need to do more for now.
Previously, GitHub itself did not support leaving reactions on Pull Request reviews. It was only possible to leave reactions on normal comments and inline review comments. They've added that feature now, so this change implements it accordingly in this package. Reference: https://developer.github.com/v4/changelog/2019-01-11-schema-changes/.
In Gerrit, it's possible to publish inline comments on patchset push. Such messages look like this: Uploaded patch set 2. (3 comments) Parse and display such comments.
Use the more explicit project~id change ID format. It's not deprecated, and lets us detect and return a not exist error when the project doesn't match the CL. Previously, just the numeric change ID was used. That format is deprecated, and doesn't tell us when the project doesn't match the CL. References: • https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id • https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#change.api.allowedIdentifier
The issue https://golang.org/issue/21358 has been resolved upstream, so there's no more need to use a temporary fork of the ctxhttp package.
Still manually crafted in order to prototype and learn about this space. It's still not completely clear how to best integrate patch sets vs multiple commits, the commit messages, and how git client fits into the picture. The end result can be seen at https://dmitri.shuralyov.com/gpu/mtl/...$changes/1.
Also remove commented out sort code. It's normal and expected for Gerrit IDs not to line up with created times. As documented at https://godoc.org/golang.org/x/build/maintner#GerritCL.Number: > Number is the CL number on the Gerrit server (e.g. 1, 2, 3). Gerrit CL > numbers are sparse (CL N does not guarantee that CL N-1 exists) and > Gerrit issues CL's out of order - it may issue CL N, then CL (N - 18), > then CL (N - 40).
This helps with displaying large PRs that have over 100 timeline items (e.g., https://github.com/neelance/go/pull/7). The implementation is based the similar issues service ListTimeline, see https://github.com/shurcooL/issues/commit/4081aa59e957752abbfc6b6fdab72c3bbc1c1dec.
Previously, the review state wasn't correctly parsed when there was more than 1 label (e.g., "Run-TryBot+1 Code-Review+2"). Example of an affected review is https://go-review.googlesource.com/c/go/+/112935#message-45e0d04e5f087880438b34c77b6b683f0beffaa3.
We need to fetch messages in Get method, so that counting them works. Previously, len(chg.Messages) was always zero, and so was Replies as a result. Make project helper implementation simpler by making its error handling path indented, and the normal execution path not indented.
Make it more consistent across the services.
A CL is considered to not exist if it's private or if it has an empty status (possible due to an upstream bug). Updates https://golang.org/issue/22060.
This is more robust. It also removes the need to talk to real GitHub API while creating a new service, which is undesirable during testing. Analogous to https://github.com/shurcooL/issues/commit/adf13e46e3d966ef14ee08722b56b823cf5c3f3b.
Previously, it panicked. Now that I have a better how to deal with it, add a comment describing the state. For now, skip such reviews. In the future, can display them with a pending/draft style.
Similar to https://github.com/shurcooL/issues/commit/697ee8e877735feed6151525cad859327bfe40c4. Follows https://dmitri.shuralyov.com/state/...$commit/28bcc343414c6adcd7b6911f9d0ef1ad6fbf30ae and https://github.com/shurcooL/events/commit/25a0212309664cda69930a58f8369f24b9029812.
Change timeline item ID type from uint64 to string.
Add unparsed messages as simple comments. This way, value of Replies corresponds to actual number of messages.
Page size 100 is largest allowed. Add full pagination later if/when needed.
It can't be set in Go 1.10 due to reflect changes. Also switch from githubqlActor to githubqlUser because RequestedReviewer union is known to be User or Team, so Actor interface is unnecessarily broad. References: - https://tip.golang.org/doc/go1.10#reflect - https://developer.github.com/v4/union/requestedreviewer/ - https://developer.github.com/v4/interface/actor/
Detect ghost actors/users via them missing entirely, rather than checking for zero DatabaseID. This is consistent with how GitHub GraphQL returns them at this time. Fix minor miscounting issue in reactions method. Remove unneeded error return parameter from reactions method. These changes help keep the code more consistent with issues/githubapi package.
It's needed because in some meta-services that unify multiple underlying services, the ThreadType changes depending on the RepoSpec. Similar to https://github.com/shurcooL/issuesapp/commit/60bf33177c6943f244f14f81f145bc8b6c63f6cc.
There's no longer a clean distinction between comments and events. There are other timeline item types that don't fit into either category cleanly. Also, underlying implementations for various APIs are very similar and have overlap. As a result, it makes sense to merge these two methods into one.
Remove ListCommitsOptions from ListCommits. Expected usage is to always get all commits, then find the one you're interested in. This is what's needed to implement prev/next commit buttons, and more closely matches what's returned by 3rd party APIs. The button styling can be improved; to be done later.