2 ms·

The issue serves no purpose for me, so here's a shorter version: Add `tables` argument to schema command Optionally restricts which tables are shown

by thunky 4y ago

js2 4y ago
I think the issue was originally an outside feature request and adding "Closes #xxx" is there just to link the commit to the issue and automatically close it.
- thunky 4y ago
  It wasn't, actually. simonw created the issue himself and self-closed it 8 minutes later. I had a conversation with simonw yesterday about why I personally view this as an anti-pattern: https://news.ycombinator.com/item?id=33388369 https://news.ycombinator.com/item?id=33388369
  - seadan83 4y ago
    I really love this specific thread, thank you all for the thoughtful commentary. An experience that is really burned into me was working on a team that put very little into commit comments or PR descriptions. But, they did have debates and design decision conversations within PR review comments. Hence the context was in the PR comments. One fine day, I was looking for the background behind some code while tracking down a problem. After identifying about 7 commits (with pretty basic/useless messages, and no PR link!), I then had to find the corresponding PRs based on timestamps, and search the PR history for PRs merged around those timestamps. From there, i had to review the conversations, some very lengthy back and forth conversation threads within the PRs to happily finally find a conversation that had the desired background. Fhe background was something like "why did you do this? X reason. Response: ok" Given that was the main thrust of the update, had that context been in the commit, it would have saved a solid hour. From this experience and others tracking down N jira items to gain context into N possible commits, my rule of thumb is that it scales poorly if you have to go to an external tool to gain context. The above experience meant tracking down the context of any one change was generally a 15 to 60 minute exercise! No chance to grep logs, just a lot of timestamps searches and scrolling through PR conversations. My preferred approach is that a first commit is "for the history." Any further commits are for the reviewer and when squashed those commit comment will generally be removed. So things like "address concern for X" and "formatting" makes sense for a reviewer coming back to an already reviewed PR, but is deleted fir someone that will look at it squashed. The idea being the squashed commit is on master, at that point nobody cares about a "formatting" commit that is one line in a series of 10 other similar commits (that person is tracking down a bug or trying to figure out non-obvious code, they do not care about renamed variables being apart from other tweaks and formatting) Hence, there is different granularity in commits, but eventually the PR and commit granularity become the same. I thus treat PRs as ephemeral with all of the initial PR text auto-generated from the commits. So, any long work to fill out a PR description instead is captured in the commit message which then autopopulates the PR description leaving nothing extra in the PR. Then any review comments are preferably not addressed directly in the PR, but with a preference to update the code for clarity or with needed documentation. Once that is done, the response to the reviewer is a "updated in git Sha X, WDYT?" Hence, the PR is for the review process, not documentation. Of course, some updates need several commits to be grok-able in review. In this case PRs are broken up, a leading PR might be 10 refactoring commits which is then squashed together with title "refactoring X", and the lusg of commits then automatically squashed into a list of reactors done in that commit. Thus avoids a very long string of commits in the history. Then the 11th commit that is the actual bug fix or functionality changes is submitted and then exists in the history as it's own commit. So the history would be like "simplify login moodule\n[list of refactors", and commit two: "fix empty username login problem\n[description of why this is a fix]"
    - thunky 4y ago
      > After identifying about 7 commits (with pretty basic/useless messages, and no PR link!), I then had to find the corresponding PRs based on timestamps, and search the PR history for PRs merged around those timestamps. Not sure if this would save any time, but it is possible to search PRs by commit. For example, say git blame led me to this commit: https://github.com/simonw/sqlite-utils/commit/129141572f249ea290e2a075437e2ebaad215859 https://github.com/simonw/sqlite-utils/commit/129141572f249e... I could have found PR #373 via this search: https://github.com/simonw/sqlite-utils/pulls?q=bb16f52681b6d35b048beb9bbf4ecc3546ef7638 https://github.com/simonw/sqlite-utils/pulls?q=bb16f52681b6d... > I thus treat PRs as ephemeral I think I see what you're saying but as others have pointed out, sometimes you want to add screenshots etc to the context, and you can't capture this kind of info in commit messages. So then you have two choices: issues or PRs. > Then any review comments are preferably not addressed directly in the PR I would think that sometimes you really do want to have a back and forth conversation in the PR, rather than just a "make this change" -> "ok done" type of feedback loop. I view the PR as an decent place for all of this because it's basically a commit of commits, capturing the related changes/conversation/context all in a single place at the point of merge.
      - seadan83 4y ago
        > I would think that sometimes you really do want to have a back and forth conversation in the PR, rather than just a "make this change I did not intend to imply that back and forth conversations would not happen. Instead, the preference would be to first address the code. Sometimes it's not clear, there are difficulties, different options, all worth a conversation. My ultimate thesis is that I think commits linking to external sources does not scale well when doing an investigation into code history. I suppose a person could do links to images in commit messages, I certainly do put links in commit comments. Though, I would agree regarding having images in PR is good. When reviewing UI changes, or making UI changes, the before and after pictures (in a PR) are super helpful. Though, again, I view the PR and its content as "for the reviewer" and not for the "bug investigator". Maybe it would be a cool git feature to be able to view images in-situ. Overall, I don't think this one weakness changes the picture of going to an external too for N commits scales very poorly. I feel it is very similar to the movement that combined code and documentation together (very long ago, api docs like javadoc used to be in different places). All that is to say, centrality of information is key IMHO for efficiency when looking at commits at scale PS: thanks for the pointer on commit search =D I was not aware of it

The issue serves no purpose for me, so here's a shorter version: Add `tables` argument to schema command Optionally restricts which tables are shown

by thunky 4y ago

The issue serves no purpose for me, so here's a shorter version: Add `tables` argument to schema command Optionally restricts which tables are shown

js2 4y ago

I think the issue was originally an outside feature request and adding "Closes #xxx" is there just to link the commit to the issue and automatically close it.

thunky 4y ago
It wasn't, actually. simonw created the issue himself and self-closed it 8 minutes later. I had a conversation with simonw yesterday about why I personally view this as an anti-pattern: https://news.ycombinator.com/item?id=33388369 https://news.ycombinator.com/item?id=33388369
- seadan83 4y ago
  I really love this specific thread, thank you all for the thoughtful commentary. An experience that is really burned into me was working on a team that put very little into commit comments or PR descriptions. But, they did have debates and design decision conversations within PR review comments. Hence the context was in the PR comments. One fine day, I was looking for the background behind some code while tracking down a problem. After identifying about 7 commits (with pretty basic/useless messages, and no PR link!), I then had to find the corresponding PRs based on timestamps, and search the PR history for PRs merged around those timestamps. From there, i had to review the conversations, some very lengthy back and forth conversation threads within the PRs to happily finally find a conversation that had the desired background. Fhe background was something like "why did you do this? X reason. Response: ok" Given that was the main thrust of the update, had that context been in the commit, it would have saved a solid hour. From this experience and others tracking down N jira items to gain context into N possible commits, my rule of thumb is that it scales poorly if you have to go to an external tool to gain context. The above experience meant tracking down the context of any one change was generally a 15 to 60 minute exercise! No chance to grep logs, just a lot of timestamps searches and scrolling through PR conversations. My preferred approach is that a first commit is "for the history." Any further commits are for the reviewer and when squashed those commit comment will generally be removed. So things like "address concern for X" and "formatting" makes sense for a reviewer coming back to an already reviewed PR, but is deleted fir someone that will look at it squashed. The idea being the squashed commit is on master, at that point nobody cares about a "formatting" commit that is one line in a series of 10 other similar commits (that person is tracking down a bug or trying to figure out non-obvious code, they do not care about renamed variables being apart from other tweaks and formatting) Hence, there is different granularity in commits, but eventually the PR and commit granularity become the same. I thus treat PRs as ephemeral with all of the initial PR text auto-generated from the commits. So, any long work to fill out a PR description instead is captured in the commit message which then autopopulates the PR description leaving nothing extra in the PR. Then any review comments are preferably not addressed directly in the PR, but with a preference to update the code for clarity or with needed documentation. Once that is done, the response to the reviewer is a "updated in git Sha X, WDYT?" Hence, the PR is for the review process, not documentation. Of course, some updates need several commits to be grok-able in review. In this case PRs are broken up, a leading PR might be 10 refactoring commits which is then squashed together with title "refactoring X", and the lusg of commits then automatically squashed into a list of reactors done in that commit. Thus avoids a very long string of commits in the history. Then the 11th commit that is the actual bug fix or functionality changes is submitted and then exists in the history as it's own commit. So the history would be like "simplify login moodule\n[list of refactors", and commit two: "fix empty username login problem\n[description of why this is a fix]"
  - thunky 4y ago
    > After identifying about 7 commits (with pretty basic/useless messages, and no PR link!), I then had to find the corresponding PRs based on timestamps, and search the PR history for PRs merged around those timestamps. Not sure if this would save any time, but it is possible to search PRs by commit. For example, say git blame led me to this commit: https://github.com/simonw/sqlite-utils/commit/129141572f249ea290e2a075437e2ebaad215859 https://github.com/simonw/sqlite-utils/commit/129141572f249e... I could have found PR #373 via this search: https://github.com/simonw/sqlite-utils/pulls?q=bb16f52681b6d35b048beb9bbf4ecc3546ef7638 https://github.com/simonw/sqlite-utils/pulls?q=bb16f52681b6d... > I thus treat PRs as ephemeral I think I see what you're saying but as others have pointed out, sometimes you want to add screenshots etc to the context, and you can't capture this kind of info in commit messages. So then you have two choices: issues or PRs. > Then any review comments are preferably not addressed directly in the PR I would think that sometimes you really do want to have a back and forth conversation in the PR, rather than just a "make this change" -> "ok done" type of feedback loop. I view the PR as an decent place for all of this because it's basically a commit of commits, capturing the related changes/conversation/context all in a single place at the point of merge.
    - seadan83 4y ago
      > I would think that sometimes you really do want to have a back and forth conversation in the PR, rather than just a "make this change I did not intend to imply that back and forth conversations would not happen. Instead, the preference would be to first address the code. Sometimes it's not clear, there are difficulties, different options, all worth a conversation. My ultimate thesis is that I think commits linking to external sources does not scale well when doing an investigation into code history. I suppose a person could do links to images in commit messages, I certainly do put links in commit comments. Though, I would agree regarding having images in PR is good. When reviewing UI changes, or making UI changes, the before and after pictures (in a PR) are super helpful. Though, again, I view the PR and its content as "for the reviewer" and not for the "bug investigator". Maybe it would be a cool git feature to be able to view images in-situ. Overall, I don't think this one weakness changes the picture of going to an external too for N commits scales very poorly. I feel it is very similar to the movement that combined code and documentation together (very long ago, api docs like javadoc used to be in different places). All that is to say, centrality of information is key IMHO for efficiency when looking at commits at scale PS: thanks for the pointer on commit search =D I was not aware of it