Skip to content
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

Remove the requirement to mention all pull requests where the problem was fixed. #295

Closed
driver733 opened this issue Feb 21, 2018 · 18 comments

Comments

@driver733
Copy link

driver733 commented Feb 21, 2018

From the policy.

I believe that it is unnecessary to mention PRs in messages, because the PR shows up automatically in the issue, when it is referenced in the commit message. So, we should change the requirement to the following:"Reference an issue in all commits"

@0crat
Copy link
Collaborator

0crat commented Feb 21, 2018

@yegor256/z please, pay attention to this issue

@llorllale
Copy link
Contributor

@driver733 I agree on the idea of reviewing the quality of the commits. What I HOPE however is that this does not lead the DEV to have to go back and fix his commits!

@driver733
Copy link
Author

@llorllale If we review commit messages as part of the PR review, we can deduct 5 points if the DEV does not reference an issue in all of his commit messages.

@llorllale
Copy link
Contributor

llorllale commented Feb 23, 2018

@driver733 sure, but after seeing the way QA reviews are done I'm afraid that QA would request and wait for the DEV to fix things before concluding with the review

@driver733
Copy link
Author

@llorllale The DEVs will the points deductions and will be more careful with their commit messages in the future. (Since commit messages cannot be changed)

@llorllale
Copy link
Contributor

@driver733 yes, I'm OK with just taking the penalty and moving on. Not sure if the ARC would see it that way.

@amihaiemil
Copy link
Contributor

@driver733 I think it should be exactly the other way around: reference the Issue only in the PR's description once, and don't reference it in the commit messages at all, because:

  1. Nobody looks in the commit messages.

  2. It polutes the Issue, because Github shows all the commits links where the issue was mentioned.

@driver733
Copy link
Author

@amihaiemil I disagree.

  1. Although nobody looks at the commit messages directly, I believe that it is important for them to appear in the issue they are associated with (I will explain that in detail in my second point)
  2. I think that the issues benefit from the commit messages coming up when the issue is referenced.
    Here is an example. There is a discussion in the ticket which leads to a certain conclusions and people to make a small change in the PR associated with the ticket. The PR assignee makes a change and references the issue in the commit message, with a short description made this small change. Then this commit pops up in the ticket and makes the ticket discussion more cohesive and understandable.

Lastly, @yegor256 advocates this himself.

Reference Tickets in Commits. Needless to say, every commit must have a message. Commits without messages are a very dirty practice; I won't even discuss why. But just a message is not enough. Every message must start with the number of the ticket you're working with. GitHub (I'm sure you are using it) will automatically link commits and tickets, increasing traceability of changes.

@t-izbassar
Copy link

@driver733 the thing is that those GitHub mentions might be with just some connected issues or even irrelevant ones for the task. For example someone might link issue from another PR that is related to 0crat, and not to the issue of the project itself. That's why it is important to have a concrete link from comments saying concrete PR, where problem was actually solved. GitHub links might be misleading and will decrease quality of the issue.

Saying that, I think there should be a rule that every commit in the PR should reference issue. But the old rule with linking PR from issue in the comments should remain as well.

@amihaiemil
Copy link
Contributor

@driver733 Well, I still think they are not so important. Because nobody is going to dig deep into them anyway. Everybody is going to look at the final form of the PR. The last "Files changed"... and if they want to study that PR in more detail, they can check the commit history there, directly -- no need to polute the discussion in the Issue with 50 links to various commits.

PS. The fact that Yegor advocates for it doesn't necessarily mean it's OK. Yegor advocates for many things

Further, the ZC improvements we're discussing here should apply for all projects, not only Yegor's -- this is one of Yegor's requirements, it may not be a general requirement by all future POs. I think most would agree with me :)

@t-izbassar
Copy link

@amihaiemil I completely disagree. You are talking about the PR only, but there is a git history with commits and their messages. And it is really important to see why change was made this way - and the answer will be in the issue discussion or in the PR discussion. That's why it is important to have PR link from issue and also issue links in commits.

@amihaiemil
Copy link
Contributor

amihaiemil commented Feb 28, 2018

@t-izbassar When was the last time you checked the whole commit history of a git repo? Personally, I can't even read the graphical representation of Eclipse, let alone understand the wall of text from the terminal?

You may be interested in the last commit maybe, then you see where the user made it, on what branch, and follow to the PR -- we have the requirement that each changes be done on a branch named after the ticket. E.g. ticket zerocracy/farm#23, is done on amihaiemil/farm branch 23.

@amihaiemil
Copy link
Contributor

@t-izbassar I think if you really must follow the whole git history, without being able to find a PR, the project has many more problems that need to be solved, not just linking Issues in commit messages. If it's so chaotic that you need to look into it in the first place, then those links won't help you, you won't know where to follow next...

@t-izbassar
Copy link

@amihaiemil don't assume what I might be interested in or not. That's my concerns and not only mine.

The common usage of commit links is when you want to know how this particular task was done. For example you have issue 123 and for some reason you want to know how it was done in the codebase level. What was used to achieve that, how was docker image create, how was integration test added - whatever. Then you just put this issue number in git history and see all the commits and can see the changes in particular commit. But, I don't want to add anything more to that topic.

@yegor256
Copy link
Member

@driver733 looks like we have to make our policy even stricter and ask the QA to check the presence of links to the issue in all commits. I like it. Any serious objections?

@driver733
Copy link
Author

@yegor256 I fully support this and believe that that is the way it should be.

@yegor256
Copy link
Member

@0crat
Copy link
Collaborator

0crat commented Mar 14, 2018

The job is not in WBS, won't close the order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants