Where communities thrive

Join over 1.5M+ people
Join over 100K+ communities
Free without limits
Create your own community

Explore more communities

Reviewable/Reviewable

Support for Reviewable

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

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

The PR is here: https://reviewable.io/reviews/batfish/batfish/5742#-

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. :)