Technical Debt vs. Precedence vs. Velocity
Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
Things like:
1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
2) Use of sdbus instead of sdbusplus
3) OEM code in common libraries
4) Lack of proper error handling
5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
6) Use of printf/fprintf instead of log<>
I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
If someone contributes and claims that their newly added technical debt is okay because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.
What say the community members on this?
--Vernon
Quoted 12 lines · Vernon Mauery
Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
Things like:
1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
2) Use of sdbus instead of sdbusplus
3) OEM code in common libraries
4) Lack of proper error handling
5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
6) Use of printf/fprintf instead of log<>I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
Quoted 8 lines
If someone contributes and claims that their newly added technical debt is okay because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.
What say the community members on this?
--Vernon
I suppose this problem is more pronounced with the ipmi repos, and I think the reason is most of that code was written much before the current set of coding practices and guidelines have been established.
While in some instances I've posted comments asking the author to rework their commits based on the current standards, it's most of the times not feasible because of the mixing coding guidelines problem you brought out, which I agree is odd to do. At the same time, I'm not sure how apt it is to ask a contributor to clean up the file in question first, if that's even possible to do at a file level, when their commit is intended to solve a specific problem and it does that job.
Sorry I don't have any answers for this right away, the one thing I can think of is planned effort to refactor the ipmi code base to meet our standards. I kind of feel that might lead to better results than cleaning up/refactoring on the go.
Thanks,
Deepak
Quoted 13 lines · Vernon Mauery
Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
Things like:
1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
2) Use of sdbus instead of sdbusplus
3) OEM code in common libraries
4) Lack of proper error handling
5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
6) Use of printf/fprintf instead of log<>
Use (your maintainer) discretion, but I think precedence would _rarely_ be a valid justification for new technical debt.
Our coding style, frameworks, guidelines, etc are all going to change over time. The codebase will never be up to date in terms of the current mode of thinking. So I would advocate for mixing code that does meet the current guidelines with code that does not, otherwise it will just get progressively worse.
Quoted 3 lines
I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
If someone contributes and claims that their newly added technical debt is okay
Maybe just write down the rules you want, and put them in a README in the project repos you maintain. Then you can simply point at this when these come up in review. Just make sure your README files get a good review as well.
I suspect some will want things like this documented at an OpenBMC wide level and not a per repository basis. I’m not sure if I like that or not. Maintaining is hard, and we all have our different things that make it less hard for us to maintain.
Quoted 3 lines
because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.
I think this is OK as long as the patch submitter has another option - adding code that meets the current guidelines without requiring the fix-up. If they have a personal issue with mixed styles, they can make the fix-up prior to their edits.
Quoted 4 lines
What say the community members on this?
--Vernon
Quoted 12 lines · Brad Bishop
Quoted 12 lines · Vernon Mauery
Overall, the openbmc project has done a great job of keeping common things in common repositories and machine or OEM specific things in separate repositories. But there are a few (because it happens) places where technical debt has accrued. I want to support getting this sort of stuff fixed so we can have a shiny project, but at what expense? Velocity. Reality calls and I was not scoped to fix old code, only to write a new feature. I am not the only one running into this issue, so I thought it best to start a discussion.
Things like:
1) Incorrect coding style (whitespace, braces, upper/lower/snake/camel)
2) Use of sdbus instead of sdbusplus
3) OEM code in common libraries
4) Lack of proper error handling
5) Use of insecure functions without proper range checking (memcpy, strcpy, etc.)
6) Use of printf/fprintf instead of log<>
Quoted 8 lines
Use (your maintainer) discretion, but I think precedence would _rarely_ be a valid justification for new technical debt.
Our coding style, frameworks, guidelines, etc are all going to change over time. The codebase will never be up to date in terms of the current mode of thinking. So I would advocate for mixing code that does meet the current guidelines with code that does not, otherwise it will just get progressively worse.
I agree with Brad's suggestion that mixing code with different style is fine. I have come across this debate a number of times in code reviews. Developers would be put in a dilemma if they have to switch styles with code in the current file. Its time to have a consensus on this aspect at project level.
Quoted 25 lines
Quoted 4 lines
I am sure there are many other classes of technical debt that I did not list here. But the point is that the code base does have places where this sort of stuff exists and has not yet been fixed. What should the official stance be? To me, mixing coding styles in a single file is worse than having the same wrong coding style in a file. But the best is if the whole project matches.
If someone contributes and claims that their newly added technical debt is okay
Maybe just write down the rules you want, and put them in a README in the project repos you maintain. Then you can simply point at this when these come up in review. Just make sure your README files get a good review as well.
I suspect some will want things like this documented at an OpenBMC wide level and not a per repository basis. I’m not sure if I like that or not. Maintaining is hard, and we all have our different things that make it less hard for us to maintain.
Quoted 3 lines
because of precedence, we don't really have a way to hold them to coming back and fixing the problem.
One way to fix this is to have contributors that want to claim precedence go and clean up the file FIRST and then come back with a new clean commit. But that does make for a higher bar for entry in some cases and possibly slows down velocity.
I think this is OK as long as the patch submitter has another option - adding code that meets the current guidelines without requiring the fix-up. If they have a personal issue with mixed styles, they can make the fix-up prior to their edits.
Quoted 3 lines
What say the community members on this?
--Vernon