No trouble canceling a sub. You don't even need to provide a card, and you'll be able to complete any reviews in progress (but not start new ones). As for permissions, I'm not sure what GitHub's UI du jour is, but if you revoke authorization completely then Reviewable will start asking for it piece-by-piece again so you can get back to the desired state.
(Let me double-check that a subscription will let you turn off the badge in public repos, though -- that was my intent but it's a corner case that doesn't get exercised often.)
Shoot, the current code will always force the badge on in public repos. Is that an issue, or are you OK trying things out in a private repo?
Sorry for the late response. We defined a required review workflow with Reviewable. An owner-file map was added to the completion condition script. For each changed file in a PR, we are able to find its owners if it has any and request review/approval from at least one of them. The owner-file map was built with OWNERS files in a repo. Since OWNERS file are changed regularly for some of our teams, we are looking for ways to update/sync Reviewable config more easily. We are able to detect changes in OWNERS files, update the owner-file map and inject it into the completion condition script automatically. As for the last step, we don’t know what would be the best option. We are considering Selenium but authentication might be a problem.
@qian-deng Would injecting the current contents of the
OWNERS file at the PR's base branch into your completion script solve your problem? I've been considering adding a feature to inject arbitrary file contents from the repo into the script for a while but was looking for a customer willing to work with me on it. (This would initially come with no UI and I'd just edit your desired list of files directly into the database. It would, however, automatically recompute conditions for open PRs when the base branch is updated -- including for changes to injected files.) What do you think?
That sounds good! I think you said reading files from a repo cannot be achieved in #722. How does injecting work in this case?
It's less that it cannot be achieved and more that it gets tricky with corner cases such that I wasn't comfortable making this trade-off for a core feature where a reliable alternative was already working. In your case, though, there's not really a better alternative and I'd we do hit a corner case you'll be able (and motivated!) to remedy it promptly. I guess I'm just more comfortable doing this for an advanced feature, though I'm also slowly coming around to accepting the trade-offs for #722, especially since the migration to GitHub apps might never happen at this point.
Anyway, I'll see how quickly I can get something running. Please let me know which repo(s) and which files you'd like injected (here or in private) and I'll let you know once they're available to your script.
Hi all, I've noticed a problem recently: After I submitted a review, the PR author bugged me to reply to some comments which I supposedly left unanswered. I checked them and I'm quite sure I did wrote a reply. Normally I'd think that's just me misremembering it (or accidentally deleting it to rewrite and forgetting to write a new version), but this happened 2/3 times recently on other PRs. Anyone maybe had a similar thing?
ofc there's a possibility that I just shouldn't review code that late :) but looks a bit suspicious, as such thing never happened before to me
the comment generated on GitHub is also missing these comments
this time it happened on a pretty huge PR (~3k LoC delta, 72 files), previously I don't remember
On my work account, I am trying to rename a file and then modify it, but then when I do, in separate commits, I find that my original file is deleted. Then my co-workers complain because now they have to reread all the code and they can't see what's changed from the old file with the original name.
Github keeps track of these changes, but I was wondering if Reviewable would do anything about it. I don't want to make a new branch just to rename my file just so they could review it. And I don't want to make a new branch because every branch takes a while to merge due to the long time it takes to build and test.
@mkow Thanks for the report -- I haven't heard anything similar recently (or ever?), though. I can't really think of a way where a draft gets deleted but the comment doesn't get posted either. (And 3k delta isn't particularly huge for Reviewable, BTW. :smile: ) For the generated GitHub message, though, there is some logic to check previously posted comment fingerprints to avoid posting duplicates in case the (non-atomic) process failed part way through on a previous publishing attempt. Were your suspicious publishing attempts "clean" (no errors) or did you have to work around any failures to get your comments out?
@elinxie This is likely because Reviewable's rename detection algorithm works between revisions, rather than between commits like git's (and presumably GitHub's) does. Usually this is fine, but if you exceed a certain threshold of changes after renaming and the two commits get aggregated into a single revision then Reviewable can miss the transition. I agree it would be better if we could detect at least "clean" renames at the commit level, though this won't be easy to shoehorn into Reviewable's architecture. Could you please open a feature request to track? In the meantime, a workaround is to push your branch after the rename, load the review page, and force the revision to be snapshotted (e.g., by creating and deleting a draft comment). As long as the revision is showing up in normal type (not italicized) in the file matrix, it's snapshotted and will persist when you push further change commits later. This should allow Reviewable to detect the rename consistently. Sorry for the trouble!
if no one else had such a problem then I'll just try to be more attentive when submitting reviews, maybe I'll be able to notice a pattern
OK, let me know what you find. One thing to keep an eye out for is whether the "to reply" counter in the toolbar is still red -- if so, there are still discussions that need an answer (or at least Reviewable thinks so). If you counter is grey, you publish, and your coworkers still find replies missing, then there's a bug -- whether in the "to reply" counting code, or in the actual posting of the comments.
Deletion is misdetected at r10, and for further revisions diffs aren't shown at all. In reality, no files are delited.
Ah, never mind: things are working as expected. The file wasn't deleted, but rather the merge put the current version of the file into the base branch. Reviewable thus marked the file as obsolete and stopped tracking it, since merging it will be a no-op. If the file changes again at head Reviewable will resume tracking and diffing it.
However, this shouldn't show up as a file deletion -- does Reviewable literally say somewhere that the file was deleted (red revision box, "File deleted at r10", etc.) or was that just your inference from the fact that revisions r10 and up were not tracked for the files?
Expected outcome: r10 should show up as a normal revision; I should be able to see the difference between r9 and r10, as well as beween r10 and r11.
Could someone give me some suggestions?
OK, looks like your browser is having trouble establishing a connection with the Firebase servers. I assume this was working before so let's try to figure out what changed. Did you install any new extensions recently? Was the local network recently reconfigured, perhaps with new firewall rules or a proxy? Does Reviewable work from another browser or over another network (e.g. your cellular connection)?
Another thing to try is to load Reviewable with
debug=workers appended to the URL and see if any network errors appear in the console. (The query forces all connections to be made in the page's own process.)
If these don't produce results we can dig deeper into network debugging -- let me know!
Hello! @pkaminski you will not remember me, I started using Reviewable more than two years ago and I love this product. I truly believe that it made our code better and the code review process much more pleasant so thank you for that. I even convinced the current project team to switch from Bitbucket to Github to be able to use Reviewable.
Now to my question. two years ago I wanted that multiple reviewers will be able to review the same PR and let each reviewer to manage the unread/read status of each file separately. So even if I reviewed 20 files, the second reviewer will see them as unread and will be able to update their status on its own. You guided me to update Condition function and provided me a text to copy-paste their to do that.
Can you please re-share the condition function to use to make each reviewer individual?
Hey Eran -- good to hear from you again, and thanks for championing Reviewable! I don't recall exactly what code snippet I showed you two years ago, but the good news is that we'll be addressing this use case head-on pretty soon. Take a look at Reviewable/Reviewable#817, which also links to all the other related issues, many of which have code samples if you need to hack your way around the problem right now.
@pkaminski one more question. Is it possible to instruct Reviewable to ignore changes in whitespaces like those?
Not ignore altogether, but you can hide them from diffs. The setting is pretty well hidden -- see towards the end of this section in the docs: https://docs.reviewable.io/files.html?highlight=whitespace#collapsed-sections
Hello. I was certain that Reviewable can render markdown tables. The table is rendered properly in github, but reviewable is displaying "raw" markdown.
https://github.com/ycm-core/ycmd/pull/1540#issuecomment-790884779
https://reviewable.io/reviews/ycm-core/ycmd/1540#