by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Jun 05 05:32
    pkaminski closed #740
  • Jun 05 05:32
    pkaminski commented #740
  • Jun 05 05:30
    Client build 4041 deployed
  • Jun 05 05:08
    pkaminski labeled #740
  • Jun 04 01:33
    dhalperi edited #740
  • Jun 04 01:33
    dhalperi opened #740
  • Jun 02 14:50
    dhbradshaw commented #736
  • Jun 02 12:09
    dhbradshaw commented #736
  • Jun 02 04:16
    pkaminski commented #736
  • Jun 02 04:15
    pkaminski labeled #739
  • Jun 02 01:32
    dato opened #739
  • Jun 01 06:22
    pkaminski commented #735
  • Jun 01 05:58
    pkaminski closed #738
  • Jun 01 05:58
    pkaminski commented #738
  • Jun 01 05:56
    pkaminski labeled #738
  • Jun 01 05:56
    pkaminski labeled #738
  • May 31 22:24
    leoluk edited #738
  • May 31 22:24
    leoluk opened #738
  • May 31 18:29
    pkaminski closed #737
  • May 31 18:29
    pkaminski commented #737
Amirhossein Tamjidi
@ahtamjidi
@pkaminski Thanks for the reply. A scfeen cast is definitely valuable. I understand your answer about vscode. I will dig into it and will contribute if I find a solution.
Dan Halperin
@dhalperi

This is going to be a very dumb question.

A year ago, or so, whenever GitHub added required reviews - I thought they were mainly adding a lot of friction compared to the review process we already had going in Reviewable. So I set the flag to disable github review integration entirely.

The main thing I disliked about the GitHub integration is that we have a very simple notion of approval: no open comments and all files reviewed => approved. But there was no (at the time) easy way to do that; instead users had to explicitly pay attention to and set redundantly a checkbox/radio (IIRC) about github reviews.

Now I want to be able to take advantage of CODEOWNERS or something like it - subtree-driven required reviewers with source-of-truth checked in.. Is there an easy way to do this?

Piotr Kaminski
@pkaminski
How do you want an owner / required reviewer to indicate that they've approved /reviewed the PR? Implicitly (mark files as reviewed) or explicitly (approve the PR)?
Dan Halperin
@dhalperi
Ideally, has reviewed all files and has no unresolved comments.
Could make LGTM or something like it required if that is important
Piotr Kaminski
@pkaminski
Are you OK keeping the subtree assignments encoded in your custom review condition code?
Dan Halperin
@dhalperi
Not really
that's not something that is checked in, reviewed, updatable by more than 1 person..
[afaik]
is there a way to make reviewable pull it from a folder in the repo?
Piotr Kaminski
@pkaminski
No, that's #722, and there are problems with that.
Dan Halperin
@dhalperi
/subbed :)
Piotr Kaminski
@pkaminski
There was also another proposal (that didn't get captured?) to pass the contents of extra files from the repo into the condition (e.g., an OWNERS file of some sort), but it has similar challenges.
Given your constraints and what's implemented right now, your best bet is likely to go with GitHub branch protection and buy into its review approval system.
Dan Halperin
@dhalperi
Can we limit that to only certain subtrees?
(Don't like this trend.... GitHub is slowly adding friction to Reviewable while fleshing out its competitor using APIs it won't expose.)
Piotr Kaminski
@pkaminski
I don't think so. To turn on Code Owner restrictions, you need to enable the overall "require pull request reviews before merging" setting, with a minimum "required approving reviews" of 1.
Yep. :(
Let me dig into this a bit today. I already have a notion of "metafiles" currently being used for .gitattributesand I may be able to stretch that to other predefined files so you can keep everything in Reviewable.
Dan Halperin
@dhalperi
okay. for now we have CODEOWNERS but it's not enforced, which is a strange middle ground... but I'm willing to figure out other things and it's not urgent.
Piotr Kaminski
@pkaminski
Actually, I don't think a generic "pass file contents into condition" would be useful here. The CODEOWNERS semantics are pretty complex and it wouldn't make sense to force your condition to deal with parsing that.
Even hardcoding the interpretation of this file into Reviewable would be annoying at best, since it allows using email addresses to identify users.
Looks like taking another swing at moving the condition into the repo might be more productive.
Dan Halperin
@dhalperi
(Unrelated to the above) getting a weird effect when typing a comment.
this is a draft pR
Kapture 2020-04-30 at 20.31.30.gif
macos, no animations, chrome latest or so
Piotr Kaminski
@pkaminski
Looks like the text div autosizer is going a bit crazy. No trailing spaces or anything unusual I assume?
Could you paste the content of the message here so I can see if it'll reproduce?
Dan Halperin
@dhalperi
Piotr Kaminski
@pkaminski
Woot, reproduced! I think the proximate cause is one of the quoted lines running right up to the margin.
I created #732 to capture the issue.
Dan Halperin
@dhalperi
I'm an inconsistent fuzzer, but at least I usually make good bug reports :p
Mike Ritchie
@mritchie-pr
Hi all, I had a Travis CI check fail due to an error with Travis CI and not my actual code. I restarted the build on Travis CI and it was successful the second time, but my review on Reviewable still shows the broken build. How do I update that in Reviewable?
Mike Ritchie
@mritchie-pr
Never mind, I restarted the build from Travis CI and it updated automatically. Don't panic!
Piotr Kaminski
@pkaminski
Hey @mritchie-pr -- yeah, these do update automatically but can sometimes get a bit delayed. If you (re)load the review page it will force a sync with the PR within seconds [1]. ([1] YMMV for very large reviews.)
Brennan Vincent
@umanwizard
hello, does Reviewable have an API ? Can't find anything about one in the docs
Ben Jackson
@puremourning
tter
Dan Halperin
@dhalperi

I'm having a crazy bug where comments get smaller when a blockquote is expanded. When the quote is collapsed, the box is big enough for it to be expanded; when the quote is expanded, the box is small enough for it to be collapsed.

I took screenshots of the UI and the DOM

Screen Shot 2020-05-27 at 9.47.57 PM.png
Screen Shot 2020-05-27 at 9.48.02 PM.png
Screen Shot 2020-05-27 at 9.49.14 PM.png
Screen Shot 2020-05-27 at 9.49.21 PM.png
Random notes: latest OS X, Chrome up to date, I have animations off. Survived a refresh.
Piotr Kaminski
@pkaminski
Sorry @umanwizard, I failed to notice your message and didn't get a notification. No, there's no API in the usual sense, but you can plug custom code into Reviewable using the custom review completion condition.
@dhalperi Apologies for the delay. Is the bug reproducible on different messages/reviews? Does it occur only when drafting a reply? Definitely worth opening an issue for either way. Thanks!
Dan Halperin
@dhalperi
@pkaminski - Reviewable/Reviewable#737 . Added what details I have, first time I've ever seen it and the PR has been merged. I believe, however, that it happened before I was drafting the reply; because investigating this was why I got into draft mode in the first place.
Dan Halperin
@dhalperi
The issue does seem reproducible for me; if the person you asked to look into this is having a hard time please let them @ me here :)
Dan Halperin
@dhalperi
General kudos for Reviewable (as always): I could not trick git/hub into recognizing the move in this PR, even though it was created by git mv ; edit file ; git commit. Squashing broke that. Of course Reviewable figured it out :). https://reviewable.io/reviews/batfish/batfish/5840
Piotr Kaminski
@pkaminski
Cool! Though that may be just because Reviewable does its best to ignore commits in general. :)