Re: [Glpi-dev] Modify coding methods to enhance code quality

[nini.lasson]

Hi David,

You have good ideas for GLPI but actually, for me, it's not possible for 
many reasons :


- how review the pull request?
  For me it's a complete job because you must know very well all 
framework of GLPI and we are very few to can do that
  and i agree with Olivier: this job must be done regularly in a very 
short time for bugs corrections

  So this can't do as long as we have a person only affect to this.

- what do you do during holidays?
   Developers have rights to be in holidays. So what do you do if you 
have a bug to fix and no other dev to check the pull request?


I think the pull request, actually, must only done for all changes and 
all changes must are subject to prior discussion with all core developer.


Another point important for me, before to do a commit, you must 
imperatively do manually tests (it's boring to have red write after a 
commit).

So, all dev must inevitably work in Debug mode.

For unit tests, i totally agree with you and Remi: we don't have enough 
tests and all parts can't be tested.


A thing can be done immediately: when you create an issue for a bug, 
don't close it after you commit but stay this issue opened until the 
return of the request (and, like before, close this issues just before 
the release if no return).


Don't forget also coding standard because actually, GLPI is very bad and 
in few time, the code will become like before the application of the 
coding standard.


Regards,
Yllen

Le 07/09/2016 à 11:16, David DURIEUX a écrit :

Hello,

I propose new coding methods to enhance GLPI (better code, have
tests, so less bugs):

* NEVER (so no exceptions) commit directly in the repository, always
   create Pull Requests
* All Pull Request must be linked with an issue (bugs, features...)
* All issues for features must be discussed and so made specification
   of what to do in the issue, with that, the developer will win time
   when will code it. The developer must be assigned to the issue
* All Pull Requests must be reviewed by another developer, this review
   is needed and check these points:
* Check the code, if it's coherent with the issue, coding standards
* check if there are enough tests (unit, integration). So a Pull
  Request NEED HAVE tests, if no test, refure merge and add comment
  in it
* verify travis (run tests in travis-ci.org) pass
* Once the PR is reviewed and all is OK, merge it. You DON'T MERGE for
   the developper pleasure, you merge because you think it's good for
   GLPI and code is good quality.

So yes, you think you will loose time, but if you have less bugs to fix
after because it's verified by unit tests, you win time ;)

Another point, for the release it will be better, you can release
directly or perhaps have one RC. Release process is more simple and
you can release when you want with these methods.

I will do that in many projects (with big code and very few developers)
and we win time, quality of code.

David
++

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev




___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[David DURIEUX]

Yep so 2 validations needed (a '+1' is enough) to merge is ok for all ?

David
++

Le Fri, 9 Sep 2016 11:25:09 +0200 (CEST)
Johan Cwiklinski  a écrit:

>Hello,
>
>> A fix cause less problem in PR, the only things will be the test
>> (enough test or not).
>> 
>> But an urgent bug fix not mean a release in urgency, so it can be
>> opened some hours (not some days :p)  
>
>Agree but anyways, bugfix or not, "urgent" or not ; if the PR is OK
>and has been reviewed by several developpers already, there no real
>need to wait for 1 day. Well, it's just a shortcut to make things
>happens quickly, I guess I could live without it ;)
>
>++

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Johan Cwiklinski]

Hello,

> A fix cause less problem in PR, the only things will be the test
> (enough test or not).
> 
> But an urgent bug fix not mean a release in urgency, so it can be
> opened some hours (not some days :p)

Agree but anyways, bugfix or not, "urgent" or not ; if the PR is OK and has 
been reviewed by several developpers already, there no real need to wait for 1 
day.
Well, it's just a shortcut to make things happens quickly, I guess I could live 
without it ;)

++
-- 
Johan

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Moron, Olivier]

1 day can be very long when it's an urgent bug fix


-Original Message-
From: Glpi-dev [mailto:glpi-dev-boun...@gna.org] On Behalf Of Alexandre Delaunay
Sent: Friday, September 09, 2016 10:47 AM
To: Liste de diffusion des developpeurs GLPI
Subject: Re: [Glpi-dev] Modify coding methods to enhance code quality

OK for me

- Mail original -
> De: "Johan Cwiklinski" <jcwiklin...@teclib.com>
> À: "Liste de diffusion des developpeurs GLPI" <glpi-dev@gna.org>
> Envoyé: Vendredi 9 Septembre 2016 09:35:37
> Objet: Re: [Glpi-dev] Modify coding methods to enhance code quality
> 
> Hello,
> 
> > For PR adding a feature / changing a behavior, I think we need to keep
> > it open before merging for some time (1 day ?) so other developpers can
> > comment it (e.g. https://github.com/glpi-project/glpi/pull/945)
> 
> OK for me, let's say PR will stay opened one day.
> 
> ++
> --
> Johan
> 
> ___
> Glpi-dev mailing list
> Glpi-dev@gna.org
> https://mail.gna.org/listinfo/glpi-dev

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev
___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Alexandre Delaunay]

OK for me

- Mail original -
> De: "Johan Cwiklinski" <jcwiklin...@teclib.com>
> À: "Liste de diffusion des developpeurs GLPI" <glpi-dev@gna.org>
> Envoyé: Vendredi 9 Septembre 2016 09:35:37
> Objet: Re: [Glpi-dev] Modify coding methods to enhance code quality
> 
> Hello,
> 
> > For PR adding a feature / changing a behavior, I think we need to keep
> > it open before merging for some time (1 day ?) so other developpers can
> > comment it (e.g. https://github.com/glpi-project/glpi/pull/945)
> 
> OK for me, let's say PR will stay opened one day.
> 
> ++
> --
> Johan
> 
> ___
> Glpi-dev mailing list
> Glpi-dev@gna.org
> https://mail.gna.org/listinfo/glpi-dev

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Johan Cwiklinski]

Hello,

> For PR adding a feature / changing a behavior, I think we need to keep
> it open before merging for some time (1 day ?) so other developpers can
> comment it (e.g. https://github.com/glpi-project/glpi/pull/945)

OK for me, let's say PR will stay opened one day.

++
-- 
Johan

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Remi Collet]

Le 07/09/2016 à 11:16, David DURIEUX a écrit :
> Hello, 
> 
> I propose new coding methods to enhance GLPI (better code, have
> tests, so less bugs):
> 
> * NEVER (so no exceptions) commit directly in the repository, always
>   create Pull Requests

For PR adding a feature / changing a behavior, I think we need to keep
it open before merging for some time (1 day ?) so other developpers can
comment it (e.g. https://github.com/glpi-project/glpi/pull/945)



Remi


___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Alexandre Delaunay]

Thanks for your message and suggestions.
I really appreciate.

For most of theses points, i agree.
- For systematic PR, we could add this process quickly. It concerns a few 
developers (mainly me, but also Walid, Yllen & Remi).
We pushed directly to master for convenience. Also, number of external 
contributions for fixes issues are not very common.
We could add this process after 9.1 release. It will be more easy and you need 
only a few weeks to wait.

- For pull requests linked to issues, it's already the case, most of the time 
(you'll certainly find some existing exceptions).
   All features was never pushed directly to master (only bugfixes), and for 
major ones, issues existed months before work started.
   Some with specs, other not, i agree we could enhance this part.
   I recently add new feature issues i'd like to see in the the next release. 
See "candidate for next version" milestone. It's not a freezed list.

- For code review, i could agree, if we had more implications. At the moment, 
existing issues and pr are most of the time read/reviewed/fixed only by me and 
yllen. So we will not wait a review for weeks/months (days is acceptable).
The main problem actually is the issue management. You couldn't ask for more 
reviews if only a few developers actually do the job.
If we have more reviews, i don't have any objections to add this process.

- Ok for unit tests and travis. It's a rather recent topic, so we couldn't do 
it previously, but your suggestion is the next step, so ok.

- For RC releases, i disagree, we had (for this release by the way) a lot of 
feedback. I appreciated this fact.
I did mistakes for last ones. I apologize for that. I'll make more efforts to 
avoid futures errors. It's not an excuse, but exhaustion is the main reason.

For CS mentioned by jcwi, i don't have a definite opinion.
Change rules to PSR should be a good point but it's a big work.

For git flow, same, if we have a consensus on this tool, ok.
I personally think it's a bit complex.

On the past year, your opinions are also welcome.
I think we missed things but we also added some good enhancements.
My first objective is the enhancement of the projet for all (dev and users).

Alexandre.

- Mail original -
> De: "Johan Cwiklinski" <jcwiklin...@teclib.com>
> À: "Liste de diffusion des developpeurs GLPI" <glpi-dev@gna.org>
> Envoyé: Jeudi 8 Septembre 2016 11:17:45
> Objet: Re: [Glpi-dev] Modify coding methods to enhance code quality
> 
> Hello,
> 
> > I propose new coding methods to enhance GLPI (better code, have
> > tests, so less bugs):
> > [...]
> 
> Two points I'd like to talk about :)
> 
> 1-
> And what about a clear GIT worflow?
> 
> For some projects, I've already used git-flow
> (http://nvie.com/posts/a-successful-git-branching-model/); mainly because 1/
> it suits my needs, and 2/ it comes with a git extension that makes things
> (really much) simple.
> 
> Maybe this one would no fit GLPI's needs in terms of worflow (mainly because
> it does not handle long term support branches - ie. only one stable version
> could be maintained); but I guess we should adopt at least some workflow
> rules.
> 
> 2-
> Coding standards has not been mentionned yet (if I remember well); but their
> respects could be added to the "contribution guide". Standardized code is
> more simple to read, understand and use :)
> Unfortunately, the rules that have been used until now are very specific (it
> is not PSR1, and even less PSR2, nor any existing standard provided with
> phpcs as fas as I can see). To automate CS checks, we'll have to either:
> - write a specfifc ruleset that suits our needs (a quite big specific work),
> - or change them to use something existing, that we could adapt for our needs
> finally (forcing all functions or methods to be correctly documented for
> example). That would be a quite big job as well, but less specific for
> instance.
> 
> Actual coding standards are documented here:
> https://forge.glpi-project.org/projects/glpi/wiki/CodingStandards
> 
> Regards,
> --
> Johan
> 
> ___
> Glpi-dev mailing list
> Glpi-dev@gna.org
> https://mail.gna.org/listinfo/glpi-dev

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Moron, Olivier]

Hello,

I agree with the whole, and just want to add 2 things:
1) for the assignments: it can be difficult for outside developers (like me).
2) for the tests, we should have unitary tests, but also integration tests , 
and at least functional tests. And for me these functional tests should be 
documented by the developer (or tester) as they are mainly done manually.

Regards,
Tomolimo 

-Original Message-
From: Glpi-dev [mailto:glpi-dev-boun...@gna.org] On Behalf Of Johan Cwiklinski
Sent: Wednesday, September 07, 2016 11:39 AM
To: Liste de diffusion des developpeurs GLPI
Subject: Re: [Glpi-dev] Modify coding methods to enhance code quality

Hello,

- Mail original -
> De: "David DURIEUX" <d.duri...@siprossii.com>
> À: "Liste de diffusion des developpeurs GLPI" <glpi-dev@gna.org>
> Envoyé: Mercredi 7 Septembre 2016 11:16:46
> Objet: [Glpi-dev] Modify coding methods to enhance code quality
> 
> Hello,
> 
> I propose new coding methods to enhance GLPI (better code, have
> tests, so less bugs):
> 
> * NEVER (so no exceptions) commit directly in the repository, always
>   create Pull Requests

I agree with that.

> * All Pull Request must be linked with an issue (bugs, features...)

Agree.

> * All issues for features must be discussed and so made specification
>   of what to do in the issue, with that, the developer will win time
>   when will code it. The developer must be assigned to the issue

Agree.

> * All Pull Requests must be reviewed by another developer, this review
>   is needed and check these points:
>* Check the code, if it's coherent with the issue, coding standards
>* check if there are enough tests (unit, integration). So a Pull
>  Request NEED HAVE tests, if no test, refure merge and add comment
>  in it
>* verify travis (run tests in travis-ci.org) pass

Agree, but disagree (on the tests part) :p

We should probably have tests on new features, but when it is actually 
possible; if we need to refactor some other (big) parts of code to get tests 
running, this is probably not a good idea ; and for bugfixes, we probably 
cannot ask the developper to spend all the required time to write all tests on 
some cases.

At the end, this rule will really become a MUST, but I'm not sure it is 
possible right now (talking about exeprience on some other projets where some 
parts of the code cannot be unit tested by design - or mistakes). I can be 
wrong, I'm quite new to the project ;)

On the basics, I totally agree, but maybe should we consider merging without 
tests would be possible with a (really!) good reason for now?
Of course, existing unit tests MUST be OK, in any case.

I'd like to add that developper that will take review MUST assign him/herself 
to the PR.

> * Once the PR is reviewed and all is OK, merge it. You DON'T MERGE for
>   the developper pleasure, you merge because you think it's good for
>   GLPI and code is good quality.

Agree.

> So yes, you think you will loose time, but if you have less bugs to fix
> after because it's verified by unit tests, you win time ;)
> 
> Another point, for the release it will be better, you can release
> directly or perhaps have one RC. Release process is more simple and
> you can release when you want with these methods.
> 
> I will do that in many projects (with big code and very few developers)
> and we win time, quality of code.

Review process and automated tests are really a must have; I can't disagree 
with that :)

We'll also have to write a (clear and quite simple) page explaining the whole 
process, for everyone reference.

Just my two cents,
++
-- 
Johan

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev
___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev

Re: [Glpi-dev] Modify coding methods to enhance code quality

[Johan Cwiklinski]

Hello,

- Mail original -
> De: "David DURIEUX" <d.duri...@siprossii.com>
> À: "Liste de diffusion des developpeurs GLPI" <glpi-dev@gna.org>
> Envoyé: Mercredi 7 Septembre 2016 11:16:46
> Objet: [Glpi-dev] Modify coding methods to enhance code quality
> 
> Hello,
> 
> I propose new coding methods to enhance GLPI (better code, have
> tests, so less bugs):
> 
> * NEVER (so no exceptions) commit directly in the repository, always
>   create Pull Requests

I agree with that.

> * All Pull Request must be linked with an issue (bugs, features...)

Agree.

> * All issues for features must be discussed and so made specification
>   of what to do in the issue, with that, the developer will win time
>   when will code it. The developer must be assigned to the issue

Agree.

> * All Pull Requests must be reviewed by another developer, this review
>   is needed and check these points:
>* Check the code, if it's coherent with the issue, coding standards
>* check if there are enough tests (unit, integration). So a Pull
>  Request NEED HAVE tests, if no test, refure merge and add comment
>  in it
>* verify travis (run tests in travis-ci.org) pass

Agree, but disagree (on the tests part) :p

We should probably have tests on new features, but when it is actually 
possible; if we need to refactor some other (big) parts of code to get tests 
running, this is probably not a good idea ; and for bugfixes, we probably 
cannot ask the developper to spend all the required time to write all tests on 
some cases.

At the end, this rule will really become a MUST, but I'm not sure it is 
possible right now (talking about exeprience on some other projets where some 
parts of the code cannot be unit tested by design - or mistakes). I can be 
wrong, I'm quite new to the project ;)

On the basics, I totally agree, but maybe should we consider merging without 
tests would be possible with a (really!) good reason for now?
Of course, existing unit tests MUST be OK, in any case.

I'd like to add that developper that will take review MUST assign him/herself 
to the PR.

> * Once the PR is reviewed and all is OK, merge it. You DON'T MERGE for
>   the developper pleasure, you merge because you think it's good for
>   GLPI and code is good quality.

Agree.

> So yes, you think you will loose time, but if you have less bugs to fix
> after because it's verified by unit tests, you win time ;)
> 
> Another point, for the release it will be better, you can release
> directly or perhaps have one RC. Release process is more simple and
> you can release when you want with these methods.
> 
> I will do that in many projects (with big code and very few developers)
> and we win time, quality of code.

Review process and automated tests are really a must have; I can't disagree 
with that :)

We'll also have to write a (clear and quite simple) page explaining the whole 
process, for everyone reference.

Just my two cents,
++
-- 
Johan

___
Glpi-dev mailing list
Glpi-dev@gna.org
https://mail.gna.org/listinfo/glpi-dev