New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Mentor notification fix issue#3881 #393

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@loriking
Contributor

loriking commented Sep 7, 2018

This makes the link in the mentor notification mailer point to a student's most recent iteration and adds a test to the code.

Background: Previously, when a student submitted a solution, mentors were notified with a link to their solution. However if the student submitted a new iteration, the link was for an earlier iteration.
see: #393

loriking and others added some commits Sep 7, 2018

Fix link in mentor notification mailer issue#3881.
Co-Authored-By: amaliacardenas <amaliacardenas@gmail.com>
@iHiD

The test looks sensible in terms of structure, but it's testing for the wrong link, and the code that makes it pass is therefore also wrong.

@iHiD iHiD changed the title from Mentor notification fix issue#3881 to [WIP] Mentor notification fix issue#3881 Sep 12, 2018

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Sep 12, 2018

Member

I wanted to give feedback on a meta topic here.

When working on a small team, it's sometimes OK to submit a pull request with no description and a title that alludes to what the PR is about, because everyone on the team has context about what is being worked on.

In open source, that's typically not well-received, as open source projects are large, sprawling, have thousands of people involved, most of whom do not have context about what's going on everywhere in the project.

The TL;DR of pull requests is this:

The pull request should stand alone.

In more detail, here are some high-level thoughts about pull requests:

The reviewer should be able to understand the purpose of the pull request, the problem being solved, and why you approached it in the way you did, without looking at other issues and pull requests.
You should provide links to relevant things, but they should be optional—the pull request itself should summarize all the context that is necessary to evaluate whether or not this is a change that can be pulled in.

The title should summarize the change that is being made.
The body should lay out the purpose, background, and any details that would be helpful to the reviewer to know about.

Also part of the pull request is the various tabs. I typically look at how many commits there are, how many files changed, and how many lines changed to get a sense of the size of the pull request.

pr-summary

Here the pull request is not too daunting, with only 4 lines added and one removed. Sometimes I need to review a pull request with hundreds of lines that have changed, in which case I will put it off. A lot.

One thing to notice is that when you reference the issue in the title, it doesn't link anywhere. This means that the reviewer has to guess what you're fixing. If we manually construct the link to issue 3881 in this repository, it gives us a 404, because the issue isn't in this repository (this repo doesn't have issues, and I happen to know that the issue is in exercism/exercism, but in a project with many repositories, links should link directly to what you're referencing).

So not only does the maintainer have to guess what problem "mentor notification fix" is, they also have to expend effort figuring out where the problem is described. Asking maintainers to do extra, unnecessary work does not provide the best first impression. They're going to be somewhat less charitable than if you did your utmost to make this a smooth PR for them to review.

You should link to things, but not from the title, since those links aren't clickable. Any link should be clickable, and correct.

Here's a great guide for how to write a pull request when working in the context of a company:
https://blog.github.com/2015-01-21-how-to-write-the-perfect-pull-request/

Much of it is relevant to open source, with the caveat that in open source you should typically not mention specific people or teams, unless the person you are mentioning is someone you know well. For example, I will often mention Jeremy when dealing with core Exercism stuff, or Wilken when working on the exercism/cli repository.

I think it would be worth your time to watch this video, which does a really good job of summarizing what a great PR looks like from the perspective of an overworked and overburdened open source maintainer: https://www.youtube.com/watch?v=u2xzRUYrsWA

Member

kytrinyx commented Sep 12, 2018

I wanted to give feedback on a meta topic here.

When working on a small team, it's sometimes OK to submit a pull request with no description and a title that alludes to what the PR is about, because everyone on the team has context about what is being worked on.

In open source, that's typically not well-received, as open source projects are large, sprawling, have thousands of people involved, most of whom do not have context about what's going on everywhere in the project.

The TL;DR of pull requests is this:

The pull request should stand alone.

In more detail, here are some high-level thoughts about pull requests:

The reviewer should be able to understand the purpose of the pull request, the problem being solved, and why you approached it in the way you did, without looking at other issues and pull requests.
You should provide links to relevant things, but they should be optional—the pull request itself should summarize all the context that is necessary to evaluate whether or not this is a change that can be pulled in.

The title should summarize the change that is being made.
The body should lay out the purpose, background, and any details that would be helpful to the reviewer to know about.

Also part of the pull request is the various tabs. I typically look at how many commits there are, how many files changed, and how many lines changed to get a sense of the size of the pull request.

pr-summary

Here the pull request is not too daunting, with only 4 lines added and one removed. Sometimes I need to review a pull request with hundreds of lines that have changed, in which case I will put it off. A lot.

One thing to notice is that when you reference the issue in the title, it doesn't link anywhere. This means that the reviewer has to guess what you're fixing. If we manually construct the link to issue 3881 in this repository, it gives us a 404, because the issue isn't in this repository (this repo doesn't have issues, and I happen to know that the issue is in exercism/exercism, but in a project with many repositories, links should link directly to what you're referencing).

So not only does the maintainer have to guess what problem "mentor notification fix" is, they also have to expend effort figuring out where the problem is described. Asking maintainers to do extra, unnecessary work does not provide the best first impression. They're going to be somewhat less charitable than if you did your utmost to make this a smooth PR for them to review.

You should link to things, but not from the title, since those links aren't clickable. Any link should be clickable, and correct.

Here's a great guide for how to write a pull request when working in the context of a company:
https://blog.github.com/2015-01-21-how-to-write-the-perfect-pull-request/

Much of it is relevant to open source, with the caveat that in open source you should typically not mention specific people or teams, unless the person you are mentioning is someone you know well. For example, I will often mention Jeremy when dealing with core Exercism stuff, or Wilken when working on the exercism/cli repository.

I think it would be worth your time to watch this video, which does a really good job of summarizing what a great PR looks like from the perspective of an overworked and overburdened open source maintainer: https://www.youtube.com/watch?v=u2xzRUYrsWA

@loriking

This comment has been minimized.

Show comment
Hide comment
@loriking

loriking Sep 12, 2018

Contributor

@kytrinyx Thank you for the detailed feedback. It's these meta issues that are hard to discover on one's own and I really appreciate you taking the time to provide links for further study. I will study the material and do better moving forward.

Contributor

loriking commented Sep 12, 2018

@kytrinyx Thank you for the detailed feedback. It's these meta issues that are hard to discover on one's own and I really appreciate you taking the time to provide links for further study. I will study the material and do better moving forward.

loriking added some commits Sep 13, 2018

Changed link to point to mentor/solution/iteration path'
Added iteration route in mentor namespace.
Tested to see if Rails.application.routes.url_helpers was necessary, and test failed without it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment