Reducing excessive and improperly studied commits

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Reducing excessive and improperly studied commits

taher
Hello Everyone,

I am starting this thread because of the latest unexpected release thread
[1] due to a major bug introduced by Jacques Le Roux in [2].

We had multiple discussion with Jacques, one such discussion [3] was due to
a bad commit in which I made a recommendation to stop doing bulk commits
and focus instead on slowly refactoring code and Jacopo mentioned in the
same thread that before doing bulk try-with-resources to start a new thread
and discuss this issue.

We faced multiple quality issues from improper commits. One such issue was
with respect to improperly closing streams [4] in which both Jacopo and
myself asked Jacques to revert and get a better understanding of how
streams work. Other discussions occured around committing quickly without
testing and hence crashing the system in [5] and [6].

Jacques continues with his stream of commits [7] and we continue to witness
some negative consequences accordingly. I'm not even sure we caught all
problems yet.

I think avoiding improperly studied, rushed or bulk commits is important
because such commits are:
- Requireing a lot of time from reviewers
- Difficult to review
- Lowering code quality

It is therefore my recommendation to agree as a community on reducing such
commits and to ask Jacques and other committers to follow the
review-then-commit process for large / complex commits.

WDYT?

[1] https://s.apache.org/clpW
[2] https://issues.apache.org/jira/browse/OFBIZ-9410
[3] https://s.apache.org/8Wq3
[4] https://s.apache.org/DpvM
[5] https://s.apache.org/IN2U
[6] https://s.apache.org/c8GG
[7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744 r1797743
r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
Reply | Threaded
Open this post in threaded view
|

Re: Reducing excessive and improperly studied commits

Michael Brohl-3
+1

I agree, this matches my suggestions in [1].

[1]
https://lists.apache.org/thread.html/1cc84ded035247ec439199ae0694d699fe1bcffb5d067c3ae732a0e9@%3Cdev.ofbiz.apache.org%3E

Regards,

Michael Brohl
ecomify GmbH
www.ecomify.de


Am 21.06.17 um 12:37 schrieb Taher Alkhateeb:

> Hello Everyone,
>
> I am starting this thread because of the latest unexpected release thread
> [1] due to a major bug introduced by Jacques Le Roux in [2].
>
> We had multiple discussion with Jacques, one such discussion [3] was due to
> a bad commit in which I made a recommendation to stop doing bulk commits
> and focus instead on slowly refactoring code and Jacopo mentioned in the
> same thread that before doing bulk try-with-resources to start a new thread
> and discuss this issue.
>
> We faced multiple quality issues from improper commits. One such issue was
> with respect to improperly closing streams [4] in which both Jacopo and
> myself asked Jacques to revert and get a better understanding of how
> streams work. Other discussions occured around committing quickly without
> testing and hence crashing the system in [5] and [6].
>
> Jacques continues with his stream of commits [7] and we continue to witness
> some negative consequences accordingly. I'm not even sure we caught all
> problems yet.
>
> I think avoiding improperly studied, rushed or bulk commits is important
> because such commits are:
> - Requireing a lot of time from reviewers
> - Difficult to review
> - Lowering code quality
>
> It is therefore my recommendation to agree as a community on reducing such
> commits and to ask Jacques and other committers to follow the
> review-then-commit process for large / complex commits.
>
> WDYT?
>
> [1] https://s.apache.org/clpW
> [2] https://issues.apache.org/jira/browse/OFBIZ-9410
> [3] https://s.apache.org/8Wq3
> [4] https://s.apache.org/DpvM
> [5] https://s.apache.org/IN2U
> [6] https://s.apache.org/c8GG
> [7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744 r1797743
> r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
> r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
> r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
>


smime.p7s (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reducing excessive and improperly studied commits

Sharan Foga
+1

Since this discussion thread started I've seen in a recent thread [1] where this issue has happened again so I agree something needs to be done to manage it.

Thanks
Sharan

[1] https://s.apache.org/EB1E


On 2017-06-21 13:03 (+0200), Michael Brohl <[hidden email]> wrote:

> +1
>
> I agree, this matches my suggestions in [1].
>
> [1]
> https://lists.apache.org/thread.html/1cc84ded035247ec439199ae0694d699fe1bcffb5d067c3ae732a0e9@%3Cdev.ofbiz.apache.org%3E
>
> Regards,
>
> Michael Brohl
> ecomify GmbH
> www.ecomify.de
>
>
> Am 21.06.17 um 12:37 schrieb Taher Alkhateeb:
> > Hello Everyone,
> >
> > I am starting this thread because of the latest unexpected release thread
> > [1] due to a major bug introduced by Jacques Le Roux in [2].
> >
> > We had multiple discussion with Jacques, one such discussion [3] was due to
> > a bad commit in which I made a recommendation to stop doing bulk commits
> > and focus instead on slowly refactoring code and Jacopo mentioned in the
> > same thread that before doing bulk try-with-resources to start a new thread
> > and discuss this issue.
> >
> > We faced multiple quality issues from improper commits. One such issue was
> > with respect to improperly closing streams [4] in which both Jacopo and
> > myself asked Jacques to revert and get a better understanding of how
> > streams work. Other discussions occured around committing quickly without
> > testing and hence crashing the system in [5] and [6].
> >
> > Jacques continues with his stream of commits [7] and we continue to witness
> > some negative consequences accordingly. I'm not even sure we caught all
> > problems yet.
> >
> > I think avoiding improperly studied, rushed or bulk commits is important
> > because such commits are:
> > - Requireing a lot of time from reviewers
> > - Difficult to review
> > - Lowering code quality
> >
> > It is therefore my recommendation to agree as a community on reducing such
> > commits and to ask Jacques and other committers to follow the
> > review-then-commit process for large / complex commits.
> >
> > WDYT?
> >
> > [1] https://s.apache.org/clpW
> > [2] https://issues.apache.org/jira/browse/OFBIZ-9410
> > [3] https://s.apache.org/8Wq3
> > [4] https://s.apache.org/DpvM
> > [5] https://s.apache.org/IN2U
> > [6] https://s.apache.org/c8GG
> > [7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744 r1797743
> > r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
> > r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
> > r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
> >
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Reducing excessive and improperly studied commits

Jacopo Cappellato-5
In reply to this post by taher
+1

Review Than Commit should be used more often.
I also wish that Jacques, from now on, will to be less resistant to
requests to revert his commits: the act of reverting a commit is not a
shame on the author nor a punishment but instead it is a mechanism that
helps to keep the code clean while some code changes are under scrutiny or
discussion; it also helps to but time to discuss (community discussions
take time); the commit history will be also improved because, after the
community review, the commit will represent a fully reviewed feature rather
than having one initial (disputed) commit followed by one or more commits
to fix/improve the original one.

Regards,

Jacopo

On Wed, Jun 21, 2017 at 12:37 PM, Taher Alkhateeb <
[hidden email]> wrote:

> Hello Everyone,
>
> I am starting this thread because of the latest unexpected release thread
> [1] due to a major bug introduced by Jacques Le Roux in [2].
>
> We had multiple discussion with Jacques, one such discussion [3] was due to
> a bad commit in which I made a recommendation to stop doing bulk commits
> and focus instead on slowly refactoring code and Jacopo mentioned in the
> same thread that before doing bulk try-with-resources to start a new thread
> and discuss this issue.
>
> We faced multiple quality issues from improper commits. One such issue was
> with respect to improperly closing streams [4] in which both Jacopo and
> myself asked Jacques to revert and get a better understanding of how
> streams work. Other discussions occured around committing quickly without
> testing and hence crashing the system in [5] and [6].
>
> Jacques continues with his stream of commits [7] and we continue to witness
> some negative consequences accordingly. I'm not even sure we caught all
> problems yet.
>
> I think avoiding improperly studied, rushed or bulk commits is important
> because such commits are:
> - Requireing a lot of time from reviewers
> - Difficult to review
> - Lowering code quality
>
> It is therefore my recommendation to agree as a community on reducing such
> commits and to ask Jacques and other committers to follow the
> review-then-commit process for large / complex commits.
>
> WDYT?
>
> [1] https://s.apache.org/clpW
> [2] https://issues.apache.org/jira/browse/OFBIZ-9410
> [3] https://s.apache.org/8Wq3
> [4] https://s.apache.org/DpvM
> [5] https://s.apache.org/IN2U
> [6] https://s.apache.org/c8GG
> [7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744 r1797743
> r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
> r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
> r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
>
Reply | Threaded
Open this post in threaded view
|

Re: Reducing excessive and improperly studied commits

Nicolas Malin-2
It's a difficult subject because I'm sure Jacques realized some change
with the clear aims to improve the framework with his knowledge.
When your knowledge is completed by other commiter's remark you are on
the crossway :
* revert all your work to try to continue to sharing on the resolution
to regenerate a better solution
* continue to improve an fix introduce bug with your mind evolution
The first way is reasonable but with the risk to freeze your initial
motivation if the community aren't react
The second generate more noise and disturb but escape the freeze risk.
Finally I can't have a opinion, each have advantage and inconvenient,
currently I experiment the extreme programming concept and I'm more
disturb now !
So too much commit or more review, I feel that isn't the real problem
particularity on trunk who it's acceptable to have some break days
(warn: I'm not in favor) but more on the real aims to achieve, the
reason to do that and sharing/understand why we are ready to waste some
time on it.

Cheers,
Nicolas

Le 29/06/2017 à 10:43, Jacopo Cappellato a écrit :

> +1
>
> Review Than Commit should be used more often.
> I also wish that Jacques, from now on, will to be less resistant to
> requests to revert his commits: the act of reverting a commit is not a
> shame on the author nor a punishment but instead it is a mechanism that
> helps to keep the code clean while some code changes are under scrutiny or
> discussion; it also helps to but time to discuss (community discussions
> take time); the commit history will be also improved because, after the
> community review, the commit will represent a fully reviewed feature rather
> than having one initial (disputed) commit followed by one or more commits
> to fix/improve the original one.
>
> Regards,
>
> Jacopo
>
> On Wed, Jun 21, 2017 at 12:37 PM, Taher Alkhateeb <
> [hidden email]> wrote:
>
>> Hello Everyone,
>>
>> I am starting this thread because of the latest unexpected release thread
>> [1] due to a major bug introduced by Jacques Le Roux in [2].
>>
>> We had multiple discussion with Jacques, one such discussion [3] was due to
>> a bad commit in which I made a recommendation to stop doing bulk commits
>> and focus instead on slowly refactoring code and Jacopo mentioned in the
>> same thread that before doing bulk try-with-resources to start a new thread
>> and discuss this issue.
>>
>> We faced multiple quality issues from improper commits. One such issue was
>> with respect to improperly closing streams [4] in which both Jacopo and
>> myself asked Jacques to revert and get a better understanding of how
>> streams work. Other discussions occured around committing quickly without
>> testing and hence crashing the system in [5] and [6].
>>
>> Jacques continues with his stream of commits [7] and we continue to witness
>> some negative consequences accordingly. I'm not even sure we caught all
>> problems yet.
>>
>> I think avoiding improperly studied, rushed or bulk commits is important
>> because such commits are:
>> - Requireing a lot of time from reviewers
>> - Difficult to review
>> - Lowering code quality
>>
>> It is therefore my recommendation to agree as a community on reducing such
>> commits and to ask Jacques and other committers to follow the
>> review-then-commit process for large / complex commits.
>>
>> WDYT?
>>
>> [1] https://s.apache.org/clpW
>> [2] https://issues.apache.org/jira/browse/OFBIZ-9410
>> [3] https://s.apache.org/8Wq3
>> [4] https://s.apache.org/DpvM
>> [5] https://s.apache.org/IN2U
>> [6] https://s.apache.org/c8GG
>> [7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744 r1797743
>> r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
>> r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
>> r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
>>

Reply | Threaded
Open this post in threaded view
|

Re: Reducing excessive and improperly studied commits

taher
Hi Nicolas,

Good input and good to have your perspective on this, thank you!

I started a thread last year [1] exactly to advocate for being brave
and innovative and to shift our somewhat conservative culture. This is
something I definitely hold near to my heart and I'm happy to say that
I think our culture _is_ changing as we are witnessing changes at a
rate that is probably faster than the past.

I think extreme programming does not tell you to tolerate lower
quality code. Instead it tells you don't make big upfront designs,
prefer simplicity, iterate quickly, refactor your code, apply Test
Driven Development, go for continuous integration, make small
releases, and so on. In a sense, it tells you make small continuous
improvements instead of trying to go the superhero route which usually
fails.

So I think the problem being discussed in this thread is not agility,
but rather quality; and we are trying to come up with solutions to
maintain quality. I think being experimental or innovative does not
prevent you from maintaining quality.

Now since quality might be a subjective term, I'll try to list some
characteristics or traits (please feel free to correct me):

- The code at a minimum compiles and passes all tests on your computer
before committing.
- Code is written carefully and slowly.
- Continuously refactor your code to make it cleaner and simpler
- If you are not comfortable with a piece of code, ask for opinions
before committing
- Prefer to have new fresh eyes on your code (pairing is essential in
extreme programming)
- Avoid quick copy-and-paste patterns and instead prefer refactoring
for modularity
- Quality wins over quantity
- Minimize bulk commits (let's change everything everywhere in the
code from X to Y). Usually it fails because the context is different
from X to Y

I think you get the idea right? This is what we're advocating for in
this thread. I don't think we want to slow down, quite the opposite.
We just want to maintain quality, which I think is even more important
for agility as nothing slows you down more than low quality code.

[1] https://lists.apache.org/thread.html/a336af8b3b8092aa8d17abc9b6811a2cb0fec5346ac275bc068d2744@%3Cdev.ofbiz.apache.org%3E

On Thu, Jun 29, 2017 at 3:14 PM, Nicolas Malin <[hidden email]> wrote:

> It's a difficult subject because I'm sure Jacques realized some change with
> the clear aims to improve the framework with his knowledge.
> When your knowledge is completed by other commiter's remark you are on the
> crossway :
> * revert all your work to try to continue to sharing on the resolution to
> regenerate a better solution
> * continue to improve an fix introduce bug with your mind evolution
> The first way is reasonable but with the risk to freeze your initial
> motivation if the community aren't react
> The second generate more noise and disturb but escape the freeze risk.
> Finally I can't have a opinion, each have advantage and inconvenient,
> currently I experiment the extreme programming concept and I'm more disturb
> now !
> So too much commit or more review, I feel that isn't the real problem
> particularity on trunk who it's acceptable to have some break days (warn:
> I'm not in favor) but more on the real aims to achieve, the reason to do
> that and sharing/understand why we are ready to waste some time on it.
>
> Cheers,
> Nicolas
>
>
> Le 29/06/2017 à 10:43, Jacopo Cappellato a écrit :
>>
>> +1
>>
>> Review Than Commit should be used more often.
>> I also wish that Jacques, from now on, will to be less resistant to
>> requests to revert his commits: the act of reverting a commit is not a
>> shame on the author nor a punishment but instead it is a mechanism that
>> helps to keep the code clean while some code changes are under scrutiny or
>> discussion; it also helps to but time to discuss (community discussions
>> take time); the commit history will be also improved because, after the
>> community review, the commit will represent a fully reviewed feature
>> rather
>> than having one initial (disputed) commit followed by one or more commits
>> to fix/improve the original one.
>>
>> Regards,
>>
>> Jacopo
>>
>> On Wed, Jun 21, 2017 at 12:37 PM, Taher Alkhateeb <
>> [hidden email]> wrote:
>>
>>> Hello Everyone,
>>>
>>> I am starting this thread because of the latest unexpected release thread
>>> [1] due to a major bug introduced by Jacques Le Roux in [2].
>>>
>>> We had multiple discussion with Jacques, one such discussion [3] was due
>>> to
>>> a bad commit in which I made a recommendation to stop doing bulk commits
>>> and focus instead on slowly refactoring code and Jacopo mentioned in the
>>> same thread that before doing bulk try-with-resources to start a new
>>> thread
>>> and discuss this issue.
>>>
>>> We faced multiple quality issues from improper commits. One such issue
>>> was
>>> with respect to improperly closing streams [4] in which both Jacopo and
>>> myself asked Jacques to revert and get a better understanding of how
>>> streams work. Other discussions occured around committing quickly without
>>> testing and hence crashing the system in [5] and [6].
>>>
>>> Jacques continues with his stream of commits [7] and we continue to
>>> witness
>>> some negative consequences accordingly. I'm not even sure we caught all
>>> problems yet.
>>>
>>> I think avoiding improperly studied, rushed or bulk commits is important
>>> because such commits are:
>>> - Requireing a lot of time from reviewers
>>> - Difficult to review
>>> - Lowering code quality
>>>
>>> It is therefore my recommendation to agree as a community on reducing
>>> such
>>> commits and to ask Jacques and other committers to follow the
>>> review-then-commit process for large / complex commits.
>>>
>>> WDYT?
>>>
>>> [1] https://s.apache.org/clpW
>>> [2] https://issues.apache.org/jira/browse/OFBIZ-9410
>>> [3] https://s.apache.org/8Wq3
>>> [4] https://s.apache.org/DpvM
>>> [5] https://s.apache.org/IN2U
>>> [6] https://s.apache.org/c8GG
>>> [7] r1798571 r1798566 r1798353 r1797792 r1797791 r1797790 r1797744
>>> r1797743
>>> r1797742 r1797373 r1797356 r1797222 r1797161 r1797160 r1797159 r1797158
>>> r1797155 r1797097 r1797079 r1797074 r1789045 r1788065 r1787949 r1761047
>>> r1761045 r1761023 r1759944 r1759088 r1759082 r1758951
>>>
>