7 ms·

I'm shocked that from all discussion nobody noted the root of all evil: "It was already late at night (I got carried away). I checked in my refactoring to mast

I'm shocked that from all discussion nobody noted the root of all evil: "It was already late at night (I got carried away). I checked in my refactoring to master and went to bed, proud of how I untangled my colleague’s messy code." No PR, no code review, no CI. Just a cowboy pushing to the master..

LandR 5y ago
This is just how some places work, unfortunately. We all push to main, no code reviews, no CI etc. Hell, some people here patch things by downloading the live dll, decompile, update code, recompile and stick it back to live...
- benjiweber 5y ago
  You realise that everyone pushing changes to main several times a day is the definition of CI? and it's pretty much impossible to do with code review as a gate for pushing to main. Interesting how CI has become semantically diffused to mean literally the opposite of CI: tooling and processes that delay integration and make it easier to work in isolation. https://wiki.c2.com/?ContinuousIntegration https://wiki.c2.com/?ContinuousIntegration " The most granular unit of integration should be one step of one refactor. "
  - 5e92cb50239222b 5y ago
    CI is more than that, at least in my understanding of the term. You have to have some sort of automated testing before doing the merge. We were doing the same shit (all pushing to the same branch) and it broke constantly before we introduced some tests and merges only after a successful test run on a separate CI server. Just pushing everything into the main branch without any checks is no CI, even if it technically can be called so.
    - CaveTech 5y ago
      That's `automated CI` which is now or less interchanged with just `CI`, however at face value there is no requirement that all CI be automated, it is just usually better to do so.
      - Jtsummers 5y ago
        For anything non-trivial (read: more than around 10-20k SLOC of C or equivalent; straightforward with minimal branching logic; non-critical) you cannot, in any practical sense, have CI without automation. Unless, of course, you want a garbage system. If you do the minimum things necessary to ensure the system is correct (in both verification and validation senses) and of decent quality, and you have no automation, you will not have CI because you will introduce delays between code change and deployment ranging from days (for smaller projects) to weeks or months (for significant projects). That delay means you do not have CI. And it also encourages batching many changes together so that you only have to have one test run (or a small number of test runs) which is also the opposite of CI. Now, you could bypass all those tests and reviews and just deploy it anyways. And then you'll have a shitty codebase, but you'll have CI so that's good, right?
        CaveTech 5y ago
        I'm not advocating for this from an engineering perspective, just stating that it does meet the semantic root of `CI`, regardless of whether you believe it meets _your_ threshold of "good" CI.
gchamonlive 5y ago
And using clean code wrong. Clean code is not a panacea. If people use it as such and without the correct agile rites, they should do it at their own peril, but blaming it on clean code is at the very least naive (and somewhat dishonest)
- sombremesa 5y ago
  Personally, I disagree that DRY is the same as “clean”. Clean code is merely code that has been thoughtfully structured (regardless of repetition) and is relatively transparent as to its purpose.
  - tcbasche 5y ago
    I've seen a few developers see multiple "duplicated" API formats for instance (for different API's) in the same codebase and think those are "duplicated" and then entangle two completely separate API's together. This is worse IMO
    - bluGill 5y ago
      There is accidental duplication to beware of. That is when different things looks the same. Then there is real duplication.. Only the later is bad and should be eliminated. Telling the differences is hard
      - cma 5y ago
        I'd rather look at a simple arithmaticl expression and know immediately what is going on, rather than have to make up a hard to remember name for every small expression that happens to get repeated one or more times in the name of DRY. The same applies to small bits of non-arithmatical code.
dataviz1000 5y ago
This guy was working at Facebook at the time too.
- ARandomerDude 5y ago
  Seems fine then. Move fast and break things.
  - rapsacnz 5y ago
    Huh, don't you mean Move fast and break society
tcbasche 5y ago
> No PR, no code review, no CI. Just a cowboy pushing to the master.. Let's be clear that the dangers here are "no code review" and "no CI". The others are fine in most circumstances, unless you work on some huge monolithic codebase. Smaller cross-functional teams should be able to push to master to solve problems then and there without everyone's unanimous approval, and everyone should feel enough safety to be able to make changes to the codebase they share with their team. The real "root of all evil" is why is someone working against the team from within the team?
herval 5y ago
...and getting told by a manager to revert code. The entire environment in the example doesn't sound very healthy
postalrat 5y ago
It was reviewed the next day by his boss. How is pushing to master not a form of CI?
- donatj 5y ago
  I was going to downvote you, but then you made me think - everyone pushing to a shared branch is the most continuous form of continuous integration I suppose. I worked this way for years before we moved into full branched code review. I would never go back however. That said, you should still have at least some review and automated testing before your code gets in everyone else's way.
- deeviant 5y ago
  Likely, it was their coworker who saw the changes, then reported the incident to the manager, rather than some sort of manager picking through every commit type process.
- the_arun 5y ago
  > It was reviewed the next day by his boss. Assuming Master is the "protected branch" and not kitchen sink, this sounds like - "we test only in production". Wouldn't or shouldn't reviews happen before merging to master?
drclau 5y ago
And... "it was already _late at night_" (emphasis mine). I learned a long time ago that not all hours in a day are created equal.

LandR 5y ago

This is just how some places work, unfortunately. We all push to main, no code reviews, no CI etc. Hell, some people here patch things by downloading the live dll, decompile, update code, recompile and stick it back to live...

benjiweber 5y ago
You realise that everyone pushing changes to main several times a day is the definition of CI? and it's pretty much impossible to do with code review as a gate for pushing to main. Interesting how CI has become semantically diffused to mean literally the opposite of CI: tooling and processes that delay integration and make it easier to work in isolation. https://wiki.c2.com/?ContinuousIntegration https://wiki.c2.com/?ContinuousIntegration " The most granular unit of integration should be one step of one refactor. "
- 5e92cb50239222b 5y ago
  CI is more than that, at least in my understanding of the term. You have to have some sort of automated testing before doing the merge. We were doing the same shit (all pushing to the same branch) and it broke constantly before we introduced some tests and merges only after a successful test run on a separate CI server. Just pushing everything into the main branch without any checks is no CI, even if it technically can be called so.
  - CaveTech 5y ago
    That's `automated CI` which is now or less interchanged with just `CI`, however at face value there is no requirement that all CI be automated, it is just usually better to do so.
    - Jtsummers 5y ago
      For anything non-trivial (read: more than around 10-20k SLOC of C or equivalent; straightforward with minimal branching logic; non-critical) you cannot, in any practical sense, have CI without automation. Unless, of course, you want a garbage system. If you do the minimum things necessary to ensure the system is correct (in both verification and validation senses) and of decent quality, and you have no automation, you will not have CI because you will introduce delays between code change and deployment ranging from days (for smaller projects) to weeks or months (for significant projects). That delay means you do not have CI. And it also encourages batching many changes together so that you only have to have one test run (or a small number of test runs) which is also the opposite of CI. Now, you could bypass all those tests and reviews and just deploy it anyways. And then you'll have a shitty codebase, but you'll have CI so that's good, right?
      - CaveTech 5y ago
        I'm not advocating for this from an engineering perspective, just stating that it does meet the semantic root of `CI`, regardless of whether you believe it meets _your_ threshold of "good" CI.

gchamonlive 5y ago

And using clean code wrong. Clean code is not a panacea. If people use it as such and without the correct agile rites, they should do it at their own peril, but blaming it on clean code is at the very least naive (and somewhat dishonest)

sombremesa 5y ago
Personally, I disagree that DRY is the same as “clean”. Clean code is merely code that has been thoughtfully structured (regardless of repetition) and is relatively transparent as to its purpose.
- tcbasche 5y ago
  I've seen a few developers see multiple "duplicated" API formats for instance (for different API's) in the same codebase and think those are "duplicated" and then entangle two completely separate API's together. This is worse IMO
  - bluGill 5y ago
    There is accidental duplication to beware of. That is when different things looks the same. Then there is real duplication.. Only the later is bad and should be eliminated. Telling the differences is hard
    - cma 5y ago
      I'd rather look at a simple arithmaticl expression and know immediately what is going on, rather than have to make up a hard to remember name for every small expression that happens to get repeated one or more times in the name of DRY. The same applies to small bits of non-arithmatical code.

dataviz1000 5y ago

This guy was working at Facebook at the time too.

ARandomerDude 5y ago
Seems fine then. Move fast and break things.
- rapsacnz 5y ago
  Huh, don't you mean Move fast and break society

tcbasche 5y ago

> No PR, no code review, no CI. Just a cowboy pushing to the master.. Let's be clear that the dangers here are "no code review" and "no CI". The others are fine in most circumstances, unless you work on some huge monolithic codebase. Smaller cross-functional teams should be able to push to master to solve problems then and there without everyone's unanimous approval, and everyone should feel enough safety to be able to make changes to the codebase they share with their team. The real "root of all evil" is why is someone working against the team from within the team?

herval 5y ago

...and getting told by a manager to revert code. The entire environment in the example doesn't sound very healthy

postalrat 5y ago

It was reviewed the next day by his boss. How is pushing to master not a form of CI?

donatj 5y ago
I was going to downvote you, but then you made me think - everyone pushing to a shared branch is the most continuous form of continuous integration I suppose. I worked this way for years before we moved into full branched code review. I would never go back however. That said, you should still have at least some review and automated testing before your code gets in everyone else's way.
deeviant 5y ago
Likely, it was their coworker who saw the changes, then reported the incident to the manager, rather than some sort of manager picking through every commit type process.
the_arun 5y ago
> It was reviewed the next day by his boss. Assuming Master is the "protected branch" and not kitchen sink, this sounds like - "we test only in production". Wouldn't or shouldn't reviews happen before merging to master?

drclau 5y ago

And... "it was already _late at night_" (emphasis mine). I learned a long time ago that not all hours in a day are created equal.