search

TokTok / website

The TokTok website
https://toktok.ltd/
GNU General Public License v3.0
10 stars 54 forks source link

Add a howto for community reviews using Reviewable. #36

Closed iphydf closed 7 years ago

iphydf commented 7 years ago

This change is Reviewable

fcore117 commented 7 years ago

Reviewed 11 of 11 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 11 of 11 files at r1. Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

.travis.yml, line 19 at r1 (raw file):


install:
- gem install jekyll mdl guard-livereload

Does Travis really need live reloading of the page? No code committed should even have that script uncommented.

toktok/reviews.md, line 10 at r1 (raw file):

The TokTok review process requires at least three reviews for every pull
request. One of the reviews must be from a developer. The two other reviews
can be from the developer but can also be from the community. The idea is that

"a developer,"

toktok/reviews.md, line 31 at r1 (raw file):

# Step 2: Log in to Reviewable

-   Open the Reviewable link in the "#" column on the page or on the PR

"or on GitHub in the PR description itself" maybe?

toktok/reviews.md, line 50 at r1 (raw file):

-   Now comes your review: your task is now to check whether the change makes
    sense. In this example, we see that `assoc4` was checked in an IPv6 code
    block. Now that is fixed. Seems legit :).

"Seems legit. :)"?

toktok/_layouts/default.html, line 16 at r1 (raw file):

    <script>const relative = "{{ relative }}";</script>
    {% endif %}
    <!-- Enable this when developing the site on localhost and you want to

Put these comments in the Liquid blocks so they're not present on the rendered website. {% comment %} Comment not present in production. :D {% endcomment %}

Comments from Reviewable

iphydf commented 7 years ago

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

.travis.yml, line 19 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Does Travis really need live reloading of the page? No code committed should even have that script uncommented.

It doesn't, but it's easier if you only need to add uncomment the javascript link when developing the page. If I remove this, I need to comment out the references in the Gemfile as well. It doesn't seem to add much time (still about 1 minute 50 seconds like some other page builds). Should I remove it altogether and forget about live reloading? I found it to be quite useful while writing the page.

toktok/reviews.md, line 10 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
"a developer,"

Done.

toktok/reviews.md, line 31 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
"or on GitHub in the PR description itself" maybe?

Done.

toktok/reviews.md, line 50 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
"Seems legit. :)"?

Shrug poor attempt at being funny. https://cdn.meme.am/cache/instances/folder268/60114268.jpg

toktok/_layouts/default.html, line 16 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Put these comments in the Liquid blocks so they're not present on the rendered website. `{% comment %} Comment not present in production. :D {% endcomment %}`

Done.

Comments from Reviewable

robinlinden commented 7 years ago

Still missing step1.png

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions.

.travis.yml, line 19 at r1 (raw file):

Previously, iphydf wrote…
It doesn't, but it's easier if you only need to add uncomment the javascript link when developing the page. If I remove this, I need to comment out the references in the Gemfile as well. It doesn't seem to add much time (still about 1 minute 50 seconds like some other page builds). Should I remove it altogether and forget about live reloading? I found it to be quite useful while writing the page.

I didn't realise. Leave it in. :)

toktok/reviews.md, line 50 at r1 (raw file):

Previously, iphydf wrote…
*Shrug* poor attempt at being funny. https://cdn.meme.am/cache/instances/folder268/60114268.jpg

Noo. I moved the full stop. It was funny.

Comments from Reviewable

iphydf commented 7 years ago

Should I put a screenshot of the pulls page? Seemed a bit redundant since users can just click on it, and it's on the same site.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

iphydf commented 7 years ago

Review status: 10 of 11 files reviewed at latest revision, 1 unresolved discussion.

toktok/reviews.md, line 50 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
Noo. I moved the full stop. It was funny.

Ahh, misunderstood :) restored and changed.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

toktok/reviews.md, line 40 at r3 (raw file):

    click this button, you should be able to see the changes under review.

![Step 1](static/img/reviews/step1.png)

I guess I should have made the comment about the missing image "step1.png" here.

Comments from Reviewable

iphydf commented 7 years ago

Review status: 10 of 11 files reviewed at latest revision, 1 unresolved discussion.

toktok/reviews.md, line 40 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…
I guess I should have made the comment about the missing image "step1.png" here.

Oops. Done :)

Comments from Reviewable

robinlinden commented 7 years ago
:lgtm_strong:

Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

iphydf commented 7 years ago

This needs another review after rebase.

GrayHatter commented 7 years ago

Reviewed 7 of 11 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, 2 of 2 files at r5. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 2 of 2 files at r5. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

iphydf commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

toktok/reviews.md, line 53 at r5 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
It's also helps developer catch mistakes. I've explained a simple change to someone once, and caught a nearby error that I missed the first go around.

Done.

toktok/reviews.md, line 69 at r5 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…
If for any reason you're not 100% sure about the file, leave a comment. That way even though you've seen the file, (according to reviewable) everyone knows why, or what you're waiting on.

I've thought about it for a while, but I still don't understand what information this adds. Can you rephrase this?

Comments from Reviewable

fcore117 commented 7 years ago

Reviewed 1 of 2 files at r2, 2 of 2 files at r5, 1 of 1 files at r6. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

robinlinden commented 7 years ago

Reviewed 1 of 1 files at r6. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

GrayHatter commented 7 years ago
:lgtm:

Reviewed 1 of 1 files at r6. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

toktok/reviews.md, line 53 at r5 (raw file):

Previously, iphydf wrote…
Done.

LGTM, that's exactly what I was thinking too.

toktok/reviews.md, line 69 at r5 (raw file):

Previously, iphydf wrote…
I've thought about it for a while, but I still don't understand what information this adds. Can you rephrase this?

I don't want reviewers to "green light" a file they want to ask questions on, and then not ask a question.

My worry is this language might be interpreted as, "I've seen this file, it's has major problems, but I've seen it." Because if the user doesn't leave a comment, then we have a false positive for number of reviewers for files.

Maybe reword this section to be fit more of suggested order of operations, instead of a description of how things work?

If I'm still unclear, ping me on IRC and I'll offer a reword that I think fixes the problem I have with this section.

Either way, I'm currently not blocking so I'm fine if you merge with this outstanding.

Comments from Reviewable

robinlinden commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

toktok/reviews.md, line 89 at r6 (raw file):

first one.

![Step 5](static/img/reviews/step5.png)

Turns out this file is missing too. :x Want to add it or should I open a PR deleting this line?

Comments from Reviewable

fcore117 commented 7 years ago

can this pull canceled then edited so step5.png works?