I make individual commits for addressing review comments. It's sometimes helpful to see this during further review both for existing or new reviewers. They get squashed like any others afterwards. I don't see how it would be valuable after something is merged to master that I renamed a bunch of variables, extracted a method somewhere and added one test after a reviewer commented that I missed a case.
Refactorings are sometimes done with a separate PR and sometimes not. Depends on the size. From our discussion we might have different philosophies on when to do that. And yes at our company merging and deploying a refactoring to master is totally acceptable and done frequently. Even if the ticket takes longer in the end, gets scrapped etc. the refactoring can probably stand on its own and be valuable if we extracted it to its own PR.
Association should be there through tickets. We may have a different idea of what small is and what will be in such a commit. Let's have a made up example:
You are building something that has a table view for some data. That feature is out there. But the table doesn't have sorting capabilities and no filtering and no paging. It just displays everything.
There are individual tickets for adding each of these. Sorting is a small PR because the table component you use actually has that capability. You add the flag to enable sorting and the definition for the default sort and release it.
Filtering is another ticket. Filtering is harder. The API this is based on doesn't support filtering and isn't owned by your team. If this had been part of the same feature branch and released together customers would have nothing yet. Instead they have sorting already. You decide to do filtering via API and not in the UI after fetching everything. So off to create a ticket for the other team and talking to them. Maybe you can help out and do it for them but in any case this will take a while.
While waiting for their answer you move on to Paging! Yet another ticket. You notice that your table component doesn't support infinite scrolling. You want to have that though instead of individual paging. You create two PRs. One to the table component which adds infinite scrolling. This is an individual PR that is regression tested against all the other users of it in your code base. Once that's done you merge it. Why wait? Then you PR the actual change to use infinite scrolling with your specific table. This is at this point still based on that API that doesn't do filtering so you still always retrieve all data. But with lots of data it still helps rendering speed and the API is reasonably fast even with thousands of rows.
You create a new ticket to clean this up and make it work by retrieving paged data once the API also does filtering. Or maybe you never will.
> I make individual commits for addressing review comments.
Those changes are visible when you make a force push in github since it generates a link that shows those changes.
> Association should be there through tickets.
I've seen companies change ticketing systems several times in career. Once it changes, all the old links and associations are as good as gone. But if that association is maintained via git, then that's not an issue.
I like how you first say that doing something in git itself is not needed because a random tool you use but I don't does things so that you can still see it easily and then you turn around and tell me that something is better to be visible in git history itself only because companies change tools.
Weird.
Tell me, in your career, how many times have you seen companies switch source control tools?
I have seen it many many times. In fact I've done some of these migrations for my companies in the past.
I've seen the same with ticketing systems as well. Yes you can loose history in both these transitions. I've worked with systems where either ticket or version history were not available past a certain point. Usually viewing many many years into the past was possible because people realized that historic information can be valuable. But there was a cutoff point that balances out ROI.
Unless required for regulatory purposes maybe, who really needs commit history from 15 years ago? It's cool don't get me wrong. I loved digging through commit history on code that originally was tracked via RCS on a (at the time) ~15 year old code base. And that was about 15 years ago. I'm getting old lol!
> Tell me, in your career, how many times have you seen companies switch source control tools?
Twice. Once from CVS to SVN (where people started using SVN for new projects and left existing projects in CVS). And once from SVN to git where we used tools to get the SVN history into git.
> who really needs commit history from 15 years ago?
It really depends on how old the code is. I've worked on code that was last deployed close to a decade ago and some of the git blame output was showing commit dates from 2008 (not quite 15 years ago). Unfortunately, the commit messages left something to be desired and didn't really help in terms of figuring out what the issue was).
> I loved digging through commit history on code that originally was tracked via RCS on a (at the time) ~15 year old code base. And that was about 15 years ago. I'm getting old lol!
Same here. I never got to use RCS at work, though I learned about it in school :)
Refactorings are sometimes done with a separate PR and sometimes not. Depends on the size. From our discussion we might have different philosophies on when to do that. And yes at our company merging and deploying a refactoring to master is totally acceptable and done frequently. Even if the ticket takes longer in the end, gets scrapped etc. the refactoring can probably stand on its own and be valuable if we extracted it to its own PR.
Association should be there through tickets. We may have a different idea of what small is and what will be in such a commit. Let's have a made up example:
You are building something that has a table view for some data. That feature is out there. But the table doesn't have sorting capabilities and no filtering and no paging. It just displays everything.
There are individual tickets for adding each of these. Sorting is a small PR because the table component you use actually has that capability. You add the flag to enable sorting and the definition for the default sort and release it.
Filtering is another ticket. Filtering is harder. The API this is based on doesn't support filtering and isn't owned by your team. If this had been part of the same feature branch and released together customers would have nothing yet. Instead they have sorting already. You decide to do filtering via API and not in the UI after fetching everything. So off to create a ticket for the other team and talking to them. Maybe you can help out and do it for them but in any case this will take a while.
While waiting for their answer you move on to Paging! Yet another ticket. You notice that your table component doesn't support infinite scrolling. You want to have that though instead of individual paging. You create two PRs. One to the table component which adds infinite scrolling. This is an individual PR that is regression tested against all the other users of it in your code base. Once that's done you merge it. Why wait? Then you PR the actual change to use infinite scrolling with your specific table. This is at this point still based on that API that doesn't do filtering so you still always retrieve all data. But with lots of data it still helps rendering speed and the API is reasonably fast even with thousands of rows.
You create a new ticket to clean this up and make it work by retrieving paged data once the API also does filtering. Or maybe you never will.