Re: [openstack-dev] [nova] Proposal new hacking rules

[Zane Bitter]

On 26/11/14 09:33, Louis Taylor wrote:

On Wed, Nov 26, 2014 at 08:54:35AM -0500, Jay Pipes wrote:

It's not about an equality condition.

It's about the message that is produced by testtools.TestCase.assertEqual(),
and the helpfulness of that message when the order of the arguments is
reversed.

This is especially true with large dict comparisons. If you get a message
like:

  reference: 
  actual: 

And the arguments are reversed, then you end up wasting time looking in the
test code instead of the real code for the thing that is different.

Anyway, like I said, it's not something that we can write a simple hacking
check for, and therefore, it's not something that should have much time
spent on. But I do recommend that reviewers bring it up, especially if the
patch author has been inconsistent in their usage of (expected, actual) in
multiple assertEqual() calls in their patch.


I think Nicolas's question was what made testtools choose this ordering. As far
as I know, the python docs for unittest uses the opposite ordering. I think
most people can see that the error messages involving 'reference' and 'actual'
are useful, but maybe not the fact that in order to achieve them using
testtools, you need to go against the norm for other testing frameworks.


The python docs for unittest mostly use 'first' and 'second' as the 
parameter names, and unittest doesn't distinguish between expected and 
actual in the default error messages.


That said, some of the newer assertions like assertDictEqual do use 
expected and actual, _in that order_, the same as testtools does.


The bottom line is that there are exactly two ways to do it, the entire 
world has now chosen one way and while I might otherwise have chosen 
differently for the same reasons as Nicolas, it would be absurd not to 
do it the same way.


That said, the entire discussion is moot because it can't be checked 
automatically.


cheers,
Zane.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Ben Nemec]

On 11/26/2014 07:54 AM, Jay Pipes wrote:
> On 11/26/2014 06:20 AM, Nicolas Trangez wrote:
>> On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:
>>> I think pointing out that the default failure
>>> message for testtools.TestCase.assertEqual() uses the terms
>>> "reference"
>>> (expected) and "actual" is a reason why reviewers *should* ask patch
>>> submitters to use (expected, actual) ordering.
>>
>> Is there any reason for this specific ordering? Not sure about others,
>> but I tend to write equality comparisons like this
>>
>>  if var == 1:
>>
>> instead of
>>
>>  if 1 == var:
>>
>> (although I've seen the latter in C code before).
>>
>> This gives rise to
>>
>>  assert var == 1
>>
>> or, moving into `unittest` domain
>>
>>  assertEqual(var, 1)
>>
>> reading it as 'Assert `var` equals 1', which makes me wonder why the
>> `assertEqual` API is defined the other way around (unlike how I'd write
>> any other equality check).
> 
> It's not about an equality condition.
> 
> It's about the message that is produced by 
> testtools.TestCase.assertEqual(), and the helpfulness of that message 
> when the order of the arguments is reversed.
> 
> This is especially true with large dict comparisons. If you get a 
> message like:
> 
>   reference: 
>   actual: 
> 
> And the arguments are reversed, then you end up wasting time looking in 
> the test code instead of the real code for the thing that is different.

And my argument is that you're going to have to check the test code
anyway, because without some sort of automated enforcement you can never
be sure that the test author got it right.  I don't personally see
having to open a failing unit test as a huge burden anyway - I generally
need to do that to see what the unit test is calling anyway.

That said, I'm not personally bothered by this.  I learned early on not
to trust the expected, actual ordering so it makes no difference what
the failure message is to me.  I think we could save less experienced
developers some aggravation by not claiming something that may not be
true, but if people disagree I'm not inclined to spend a bunch of time
bikeshedding about it either. :-)

-Ben

> 
> Anyway, like I said, it's not something that we can write a simple 
> hacking check for, and therefore, it's not something that should have 
> much time spent on. But I do recommend that reviewers bring it up, 
> especially if the patch author has been inconsistent in their usage of 
> (expected, actual) in multiple assertEqual() calls in their patch.
> 
> Best,
> -jay
> 
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Jay Pipes]

On 11/26/2014 09:28 AM, Nicolas Trangez wrote:

On Wed, 2014-11-26 at 08:54 -0500, Jay Pipes wrote:

On 11/26/2014 06:20 AM, Nicolas Trangez wrote:

On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:

I think pointing out that the default failure
message for testtools.TestCase.assertEqual() uses the terms
"reference"
(expected) and "actual" is a reason why reviewers *should* ask patch
submitters to use (expected, actual) ordering.


Is there any reason for this specific ordering? Not sure about others,
but I tend to write equality comparisons like this

  if var == 1:

instead of

  if 1 == var:

(although I've seen the latter in C code before).

This gives rise to

  assert var == 1

or, moving into `unittest` domain

  assertEqual(var, 1)

reading it as 'Assert `var` equals 1', which makes me wonder why the
`assertEqual` API is defined the other way around (unlike how I'd write
any other equality check).


It's not about an equality condition.

It's about the message that is produced by
testtools.TestCase.assertEqual(), and the helpfulness of that message
when the order of the arguments is reversed.


I'm aware of that. I was just wondering whether there's a particular
reason the ordering (and as a result of that the error message) was
chosen as it is.

I'd rather design the API as `assertEqual(value, expected)`, and let the
message indeed say 'Expected ..., but got ...' (and using the argument
values accordingly).


I think you'd have the same problem, no? People would still need to get 
the order of the arguments "correct".


-jay

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Louis Taylor]

On Wed, Nov 26, 2014 at 08:54:35AM -0500, Jay Pipes wrote:
> It's not about an equality condition.
> 
> It's about the message that is produced by testtools.TestCase.assertEqual(),
> and the helpfulness of that message when the order of the arguments is
> reversed.
> 
> This is especially true with large dict comparisons. If you get a message
> like:
> 
>  reference: 
>  actual: 
> 
> And the arguments are reversed, then you end up wasting time looking in the
> test code instead of the real code for the thing that is different.
> 
> Anyway, like I said, it's not something that we can write a simple hacking
> check for, and therefore, it's not something that should have much time
> spent on. But I do recommend that reviewers bring it up, especially if the
> patch author has been inconsistent in their usage of (expected, actual) in
> multiple assertEqual() calls in their patch.

I think Nicolas's question was what made testtools choose this ordering. As far
as I know, the python docs for unittest uses the opposite ordering. I think
most people can see that the error messages involving 'reference' and 'actual'
are useful, but maybe not the fact that in order to achieve them using
testtools, you need to go against the norm for other testing frameworks.

(fwiw, I'm an advocate for using the ordering with the best error messages)


signature.asc
Description: Digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Nicolas Trangez]

On Wed, 2014-11-26 at 08:54 -0500, Jay Pipes wrote:
> On 11/26/2014 06:20 AM, Nicolas Trangez wrote:
> > On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:
> >> I think pointing out that the default failure
> >> message for testtools.TestCase.assertEqual() uses the terms
> >> "reference"
> >> (expected) and "actual" is a reason why reviewers *should* ask patch
> >> submitters to use (expected, actual) ordering.
> >
> > Is there any reason for this specific ordering? Not sure about others,
> > but I tend to write equality comparisons like this
> >
> >  if var == 1:
> >
> > instead of
> >
> >  if 1 == var:
> >
> > (although I've seen the latter in C code before).
> >
> > This gives rise to
> >
> >  assert var == 1
> >
> > or, moving into `unittest` domain
> >
> >  assertEqual(var, 1)
> >
> > reading it as 'Assert `var` equals 1', which makes me wonder why the
> > `assertEqual` API is defined the other way around (unlike how I'd write
> > any other equality check).
> 
> It's not about an equality condition.
> 
> It's about the message that is produced by 
> testtools.TestCase.assertEqual(), and the helpfulness of that message 
> when the order of the arguments is reversed.

I'm aware of that. I was just wondering whether there's a particular
reason the ordering (and as a result of that the error message) was
chosen as it is.

I'd rather design the API as `assertEqual(value, expected)`, and let the
message indeed say 'Expected ..., but got ...' (and using the argument
values accordingly).

Nicolas


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Jay Pipes]

On 11/26/2014 06:20 AM, Nicolas Trangez wrote:

On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:

I think pointing out that the default failure
message for testtools.TestCase.assertEqual() uses the terms
"reference"
(expected) and "actual" is a reason why reviewers *should* ask patch
submitters to use (expected, actual) ordering.


Is there any reason for this specific ordering? Not sure about others,
but I tend to write equality comparisons like this

 if var == 1:

instead of

 if 1 == var:

(although I've seen the latter in C code before).

This gives rise to

 assert var == 1

or, moving into `unittest` domain

 assertEqual(var, 1)

reading it as 'Assert `var` equals 1', which makes me wonder why the
`assertEqual` API is defined the other way around (unlike how I'd write
any other equality check).


It's not about an equality condition.

It's about the message that is produced by 
testtools.TestCase.assertEqual(), and the helpfulness of that message 
when the order of the arguments is reversed.


This is especially true with large dict comparisons. If you get a 
message like:


 reference: 
 actual: 

And the arguments are reversed, then you end up wasting time looking in 
the test code instead of the real code for the thing that is different.


Anyway, like I said, it's not something that we can write a simple 
hacking check for, and therefore, it's not something that should have 
much time spent on. But I do recommend that reviewers bring it up, 
especially if the patch author has been inconsistent in their usage of 
(expected, actual) in multiple assertEqual() calls in their patch.


Best,
-jay

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Nicolas Trangez]

On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:
> I think pointing out that the default failure 
> message for testtools.TestCase.assertEqual() uses the terms
> "reference" 
> (expected) and "actual" is a reason why reviewers *should* ask patch 
> submitters to use (expected, actual) ordering.

Is there any reason for this specific ordering? Not sure about others,
but I tend to write equality comparisons like this

if var == 1:

instead of

if 1 == var:

(although I've seen the latter in C code before).

This gives rise to

assert var == 1

or, moving into `unittest` domain

assertEqual(var, 1)

reading it as 'Assert `var` equals 1', which makes me wonder why the
`assertEqual` API is defined the other way around (unlike how I'd write
any other equality check).

Nicolas


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Jay Pipes]

On 11/24/2014 01:02 PM, pcrews wrote:

On 11/24/2014 09:40 AM, Ben Nemec wrote:

On 11/24/2014 08:50 AM, Matthew Gilliard wrote:

1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.

2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.


Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific
ordering.


++.  It should be a part of review to ensure that the test (including
error messages) makes sense.  Simply having a (seemingly costly to
implement and enforce) rule stating that something must adhere to a
pattern does not guarantee that.


So, as a proponent of the (expected, actual) parameter order thing, I'll 
just say that I actually agree with Patrick and Ben about NOT having a 
hacking rule for this. The reason is because of what Ben noted: there's 
really no way to programmatically check for this.



assertEqual(expected, actual, msg="nom nom nom cookie cookie yum")
matches the pattern, but the message still doesn't necessarily provide
much worth.

Focusing on making tests informative and clear about what is thought to
be broken on failure seems to be the better target (imo).


Agreed. And for me, I think pointing out that the default failure 
message for testtools.TestCase.assertEqual() uses the terms "reference" 
(expected) and "actual" is a reason why reviewers *should* ask patch 
submitters to use (expected, actual) ordering. I just don't think it's 
something that can be hacking-rule-tested for...


Best,
-jay


Alternatively I suppose we could require kwargs for expected and actual
in assertEqual.  That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.



3/ warn{ing} -
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322


On the overarching point: There is no way to get started with
OpenStack, other than starting small.  My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews.  But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.

Linus' post on this from the LKML is almost a decade old (!) but
worth reading.
https://lkml.org/lkml/2004/12/20/255

   MG

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[pcrews]

On 11/24/2014 09:40 AM, Ben Nemec wrote:

On 11/24/2014 08:50 AM, Matthew Gilliard wrote:

1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.

2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.


Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific ordering.


++.  It should be a part of review to ensure that the test (including 
error messages) makes sense.  Simply having a (seemingly costly to 
implement and enforce) rule stating that something must adhere to a 
pattern does not guarantee that.


assertEqual(expected, actual, msg="nom nom nom cookie cookie yum") 
matches the pattern, but the message still doesn't necessarily provide 
much worth.


Focusing on making tests informative and clear about what is thought to 
be broken on failure seems to be the better target (imo).




Alternatively I suppose we could require kwargs for expected and actual
in assertEqual.  That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.



3/ warn{ing} - 
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322

On the overarching point: There is no way to get started with
OpenStack, other than starting small.  My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews.  But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.

Linus' post on this from the LKML is almost a decade old (!) but worth reading.
https://lkml.org/lkml/2004/12/20/255

   MG

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Ben Nemec]

On 11/24/2014 08:50 AM, Matthew Gilliard wrote:
> 1/ assertFalse() vs assertEqual(x, False) - these are semantically
> different because of python's notion of truthiness, so I don't think
> we ought to make this a rule.
> 
> 2/ expected/actual - incorrect failure messages have cost me more time
> than I should admit to. I don't see any reason not to try to improve
> in this area, even if it's difficult to automate.

Personally I'd rather kill the expected, actual ordering and just have
first, second or something that doesn't imply which value is which.
Because it can't be automatically enforced, we'll _never_ fix all of the
expected, actual mistakes (and will continually introduce new ones), so
I'd prefer to eliminate the confusion by not requiring a specific ordering.

Alternatively I suppose we could require kwargs for expected and actual
in assertEqual.  That would at least make it more obvious when someone
has gotten it backward, but again that's a ton of code churn for minimal
gain IMHO.

> 
> 3/ warn{ing} - 
> https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
> 
> On the overarching point: There is no way to get started with
> OpenStack, other than starting small.  My first ever patch (a tidy-up)
> was rejected for being trivial, and that was confusing and
> disheartening. Nova has a lot on its plate, sure, and plenty of
> pending code reviews.  But there is also a lot of inconsistency and
> unloved code which *is* worth fixing, because a tidy codebase is a joy
> to work with, *and* these changes are ideal to bring new reviewers and
> developers into the project.
> 
> Linus' post on this from the LKML is almost a decade old (!) but worth 
> reading.
> https://lkml.org/lkml/2004/12/20/255
> 
>   MG
> 
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Alexis Lee]

Matthew Gilliard said on Mon, Nov 24, 2014 at 02:50:08PM +:
> 1/ assertFalse() vs assertEqual(x, False) - these are semantically
> different because of python's notion of truthiness, so I don't think
> we ought to make this a rule.
> 2/ expected/actual - I don't see any reason not to try to improve
> in this area, even if it's difficult to automate.
> 3/ warn{ing} - 
> https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322
"N331: Use LOG.warning due to compatibility with py3"
>
> Linus' post on this from the LKML is almost a decade old (!) but worth 
> reading.
> https://lkml.org/lkml/2004/12/20/255

+1 on all points.


Alexis
-- 
Nova Engineer, HP Cloud.  AKA lealexis, lxsli.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Matthew Gilliard]

1/ assertFalse() vs assertEqual(x, False) - these are semantically
different because of python's notion of truthiness, so I don't think
we ought to make this a rule.

2/ expected/actual - incorrect failure messages have cost me more time
than I should admit to. I don't see any reason not to try to improve
in this area, even if it's difficult to automate.

3/ warn{ing} - 
https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L322

On the overarching point: There is no way to get started with
OpenStack, other than starting small.  My first ever patch (a tidy-up)
was rejected for being trivial, and that was confusing and
disheartening. Nova has a lot on its plate, sure, and plenty of
pending code reviews.  But there is also a lot of inconsistency and
unloved code which *is* worth fixing, because a tidy codebase is a joy
to work with, *and* these changes are ideal to bring new reviewers and
developers into the project.

Linus' post on this from the LKML is almost a decade old (!) but worth reading.
https://lkml.org/lkml/2004/12/20/255

  MG

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Sahid Orentino Ferdjaoui]

On Fri, Nov 21, 2014 at 10:30:59AM -0800, Joe Gordon wrote:
> On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui <
> sahid.ferdja...@redhat.com> wrote:
> 
> > On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> > > On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> > > sahid.ferdja...@redhat.com> wrote:
> > >
> > > > This is something we can call nitpiking or low priority.
> > > >
> > >
> > > This all seems like nitpicking for very little value. I think there are
> > > better things we can be focusing on instead of thinking of new ways to
> > nit
> > > pick. So I am -1 on all of these.
> >
> > Yes as written this is low priority but something necessary for a
> > project like Nova it is.
> >
> >
> Why do you think this is necessary?

Your asking make sense; You/Nova is looking for engineering time to
focus on other development more importants. I would to help with my
humble experience.

 * Let developer a chance to know about what values was expected when
   he broke a test.

 * Let developer to know what to use between warn or warning instead
   of loosing time by looking in the module what was used or doing a
   coin flip.

Contributors are expected to read HACKING.rst and some of these rules
can be tested by gate.

> > Considered that I feel sad to take your time. Can I suggest you to
> > take no notice of this and let's others developers working on Nova too
> > do this job ?
> >
> >
> As the maintainer of openstack-dev/hacking and as a nova core, I don't
> think this is worth doing at all. Nova already has enough on its plate and
> doesn't need extra code to review.

My point was not to discredit your opinion (My phrasing can be
wrong I'm non-native english) I believe that you and contributors in
general like me are working to make Nova better. Usually in
opensource software contributors are welcome to help even if it is to
fix a typo and I was hoped to help.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Sahid Orentino Ferdjaoui]

On Fri, Nov 21, 2014 at 05:23:28PM -0500, Matthew Treinish wrote:
> On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote:
> > On 11/21/2014 01:52 PM, Matthew Treinish wrote:
> > > On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> > >> Hey,
> > >> I am not a Nova developer but I still have an opinion.
> > >>
> > >>> Using boolean assertions
> > >> I like what you propose. We should use and enforce the assert* that best 
> > >> matches the intention. It's about semantic and the more precise we are, 
> > >> the better.
> > >>
> > >>> Using same order of arguments in equality assertions
> > >> Why not. But I don't know how we can write a Hacking rule for this. So 
> > >> you may fix all the occurrences for this now, but it might get back in 
> > >> the future.
> > > 
> > > Ok I'll bite, besides the enforceability issue which you pointed out, it 
> > > just
> > > doesn't make any sense, you're asserting 2 things are equal: (A == B) == 
> > > (B == A)
> > > and I honestly feel that it goes beyond nitpicking because of that. 
> > > 
> > > It's also a fallacy that there will always be an observed value and an
> > > expected value. For example:
> > > 
> > >   self.assertEqual(method_a(), method_b())
> > > 
> > > Which one is observed and which one is expected? I think this proposal is 
> > > just
> > > reading into the parameter names a bit too much.
> > 
> > If you are using assertEqual with 2 variable values... you are doing
> > your test wrong.
> > 
> > I was originally in your camp. But honestly, the error message provided
> > to the user does say expected and observed, and teaching everyone that
> > you have to ignore the error message is a much harder thing to do than
> > flip the code to conform to it, creating less confusion.
> > 
> 
> Uhm, no it doesn't, the default error message is "A != B". [1][2][3] (both 
> with
> unittest and testtools) If the error message was like that, then sure saying
> one way was right over the other would be fine, (assuming you didn't specify a
> different error message) but, that's not what it does.
> 
> 
> [1] 
> https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340
> [2] 
> https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85
>  
> [3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508

...
  File 
"/opt/stack/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 
348, in assertEqual
self.assertThat(observed, matcher, message)
  File 
"/opt/stack/nova/.venv/lib/python2.7/site-packages/testtools/testcase.py", line 
433, in assertThat
raise mismatch_error
MismatchError: !=:
reference = {'nova_object.changes': ['cells'],
 'nova_object.data': {...
actual= {'nova_object.changes': ['cells'],
 'nova_object.data': {...
...


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Sahid Orentino Ferdjaoui]

On Fri, Nov 21, 2014 at 01:52:50PM -0500, Matthew Treinish wrote:
> On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> > Hey,
> > I am not a Nova developer but I still have an opinion.
> > 
> > >Using boolean assertions
> > I like what you propose. We should use and enforce the assert* that best 
> > matches the intention. It's about semantic and the more precise we are, the 
> > better.
> > 
> > >Using same order of arguments in equality assertions
> > Why not. But I don't know how we can write a Hacking rule for this. So you 
> > may fix all the occurrences for this now, but it might get back in the 
> > future.

Let write this rules in HACKING.rst, developers and reviewers are
expected to read it.

> Ok I'll bite, besides the enforceability issue which you pointed out, it just
> doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B 
> == A)
> and I honestly feel that it goes beyond nitpicking because of that. 
> 
> It's also a fallacy that there will always be an observed value and an
> expected value. For example:
> 
>   self.assertEqual(method_a(), method_b())
> 
> Which one is observed and which one is expected? I think this proposal is just
> reading into the parameter names a bit too much.

Let developer to know about what values was expected when he broke a
test during his development without looking into the testcase code.

Operators can also want to know about what values was
expected/observed without reading the code when executing tests.

> > 
> > >Using LOG.warn instead of LOG.warning
> > I am -1 on this. The part that comes after LOG. (LOG.warning, LOG.error, 
> > LOG.debug, etc) is the log level, it's not a verb. In syslog, the 
> > well-known log level is "warning" so the correct method to use here is, 
> > imo, log.warning().

Well I have choiced 'warn' because there is less changes but I do not
have any preference, just want something clear from Nova.

> > Have you concidered submitting this hacking rules to the hacking project 
> > here : https://github.com/openstack-dev/hacking ? I am sure these new rules 
> > makes sense on other openstack projects.

Make them accepted by Nova community first before to think about other
projects ;)

> > Jordan  
> > 
> > - Original Message -
> > From: "Sahid Orentino Ferdjaoui" 
> > To: "OpenStack Development Mailing List (not for usage questions)" 
> > 
> > Sent: Friday, November 21, 2014 5:57:14 PM
> > Subject: Re: [openstack-dev] [nova] Proposal new hacking rules
> > 
> > On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> > > On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> > > sahid.ferdja...@redhat.com> wrote:
> > > 
> > > > This is something we can call nitpiking or low priority.
> > > >
> > > 
> > > This all seems like nitpicking for very little value. I think there are
> > > better things we can be focusing on instead of thinking of new ways to nit
> > > pick. So I am -1 on all of these.
> > 
> > Yes as written this is low priority but something necessary for a
> > project like Nova it is.
> > 
> > Considered that I feel sad to take your time. Can I suggest you to
> > take no notice of this and let's others developers working on Nova too
> > do this job ?
> > 
> 



> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Solly Ross]

Well, at least the message for exceptions in Nova says "expected" and 
"observed".
I suspect that it's part of our custom test case classes.

Best Regards,
Solly Ross


- Original Message -
> From: "Matthew Treinish" 
> To: "OpenStack Development Mailing List (not for usage questions)" 
> 
> Sent: Friday, November 21, 2014 5:23:28 PM
> Subject: Re: [openstack-dev] [nova] Proposal new hacking rules
> 
> On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote:
> > On 11/21/2014 01:52 PM, Matthew Treinish wrote:
> > > On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> > >> Hey,
> > >> I am not a Nova developer but I still have an opinion.
> > >>
> > >>> Using boolean assertions
> > >> I like what you propose. We should use and enforce the assert* that best
> > >> matches the intention. It's about semantic and the more precise we are,
> > >> the better.
> > >>
> > >>> Using same order of arguments in equality assertions
> > >> Why not. But I don't know how we can write a Hacking rule for this. So
> > >> you may fix all the occurrences for this now, but it might get back in
> > >> the future.
> > > 
> > > Ok I'll bite, besides the enforceability issue which you pointed out, it
> > > just
> > > doesn't make any sense, you're asserting 2 things are equal: (A == B) ==
> > > (B == A)
> > > and I honestly feel that it goes beyond nitpicking because of that.
> > > 
> > > It's also a fallacy that there will always be an observed value and an
> > > expected value. For example:
> > > 
> > >   self.assertEqual(method_a(), method_b())
> > > 
> > > Which one is observed and which one is expected? I think this proposal is
> > > just
> > > reading into the parameter names a bit too much.
> > 
> > If you are using assertEqual with 2 variable values... you are doing
> > your test wrong.
> > 
> > I was originally in your camp. But honestly, the error message provided
> > to the user does say expected and observed, and teaching everyone that
> > you have to ignore the error message is a much harder thing to do than
> > flip the code to conform to it, creating less confusion.
> > 
> 
> Uhm, no it doesn't, the default error message is "A != B". [1][2][3] (both
> with
> unittest and testtools) If the error message was like that, then sure saying
> one way was right over the other would be fine, (assuming you didn't specify
> a
> different error message) but, that's not what it does.
> 
> 
> [1]
> https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340
> [2]
> https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85
> [3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508
> 
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Solly Ross]

Whoops, that should say "assertions" not "exceptions".

- Original Message -
> From: "Solly Ross" 
> To: "OpenStack Development Mailing List (not for usage questions)" 
> 
> Sent: Monday, November 24, 2014 12:00:44 AM
> Subject: Re: [openstack-dev] [nova] Proposal new hacking rules
> 
> Well, at least the message for exceptions in Nova says "expected" and
> "observed".
> I suspect that it's part of our custom test case classes.
> 
> Best Regards,
> Solly Ross
> 
> 
> - Original Message -
> > From: "Matthew Treinish" 
> > To: "OpenStack Development Mailing List (not for usage questions)"
> > 
> > Sent: Friday, November 21, 2014 5:23:28 PM
> > Subject: Re: [openstack-dev] [nova] Proposal new hacking rules
> > 
> > On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote:
> > > On 11/21/2014 01:52 PM, Matthew Treinish wrote:
> > > > On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> > > >> Hey,
> > > >> I am not a Nova developer but I still have an opinion.
> > > >>
> > > >>> Using boolean assertions
> > > >> I like what you propose. We should use and enforce the assert* that
> > > >> best
> > > >> matches the intention. It's about semantic and the more precise we
> > > >> are,
> > > >> the better.
> > > >>
> > > >>> Using same order of arguments in equality assertions
> > > >> Why not. But I don't know how we can write a Hacking rule for this. So
> > > >> you may fix all the occurrences for this now, but it might get back in
> > > >> the future.
> > > > 
> > > > Ok I'll bite, besides the enforceability issue which you pointed out,
> > > > it
> > > > just
> > > > doesn't make any sense, you're asserting 2 things are equal: (A == B)
> > > > ==
> > > > (B == A)
> > > > and I honestly feel that it goes beyond nitpicking because of that.
> > > > 
> > > > It's also a fallacy that there will always be an observed value and an
> > > > expected value. For example:
> > > > 
> > > >   self.assertEqual(method_a(), method_b())
> > > > 
> > > > Which one is observed and which one is expected? I think this proposal
> > > > is
> > > > just
> > > > reading into the parameter names a bit too much.
> > > 
> > > If you are using assertEqual with 2 variable values... you are doing
> > > your test wrong.
> > > 
> > > I was originally in your camp. But honestly, the error message provided
> > > to the user does say expected and observed, and teaching everyone that
> > > you have to ignore the error message is a much harder thing to do than
> > > flip the code to conform to it, creating less confusion.
> > > 
> > 
> > Uhm, no it doesn't, the default error message is "A != B". [1][2][3] (both
> > with
> > unittest and testtools) If the error message was like that, then sure
> > saying
> > one way was right over the other would be fine, (assuming you didn't
> > specify
> > a
> > different error message) but, that's not what it does.
> > 
> > 
> > [1]
> > https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340
> > [2]
> > https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85
> > [3]
> > https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508
> > 
> > ___
> > OpenStack-dev mailing list
> > OpenStack-dev@lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> > 
> 

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Matthew Treinish]

On Fri, Nov 21, 2014 at 04:15:07PM -0500, Sean Dague wrote:
> On 11/21/2014 01:52 PM, Matthew Treinish wrote:
> > On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> >> Hey,
> >> I am not a Nova developer but I still have an opinion.
> >>
> >>> Using boolean assertions
> >> I like what you propose. We should use and enforce the assert* that best 
> >> matches the intention. It's about semantic and the more precise we are, 
> >> the better.
> >>
> >>> Using same order of arguments in equality assertions
> >> Why not. But I don't know how we can write a Hacking rule for this. So you 
> >> may fix all the occurrences for this now, but it might get back in the 
> >> future.
> > 
> > Ok I'll bite, besides the enforceability issue which you pointed out, it 
> > just
> > doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B 
> > == A)
> > and I honestly feel that it goes beyond nitpicking because of that. 
> > 
> > It's also a fallacy that there will always be an observed value and an
> > expected value. For example:
> > 
> >   self.assertEqual(method_a(), method_b())
> > 
> > Which one is observed and which one is expected? I think this proposal is 
> > just
> > reading into the parameter names a bit too much.
> 
> If you are using assertEqual with 2 variable values... you are doing
> your test wrong.
> 
> I was originally in your camp. But honestly, the error message provided
> to the user does say expected and observed, and teaching everyone that
> you have to ignore the error message is a much harder thing to do than
> flip the code to conform to it, creating less confusion.
> 

Uhm, no it doesn't, the default error message is "A != B". [1][2][3] (both with
unittest and testtools) If the error message was like that, then sure saying
one way was right over the other would be fine, (assuming you didn't specify a
different error message) but, that's not what it does.


[1] 
https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L340
[2] 
https://github.com/testing-cabal/testtools/blob/master/testtools/matchers/_basic.py#L85
 
[3] https://hg.python.org/cpython/file/301d62ef5c0b/Lib/unittest/case.py#l508


pgp9OcDwSQb6e.pgp
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Sean Dague]

On 11/21/2014 01:52 PM, Matthew Treinish wrote:
> On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
>> Hey,
>> I am not a Nova developer but I still have an opinion.
>>
>>> Using boolean assertions
>> I like what you propose. We should use and enforce the assert* that best 
>> matches the intention. It's about semantic and the more precise we are, the 
>> better.
>>
>>> Using same order of arguments in equality assertions
>> Why not. But I don't know how we can write a Hacking rule for this. So you 
>> may fix all the occurrences for this now, but it might get back in the 
>> future.
> 
> Ok I'll bite, besides the enforceability issue which you pointed out, it just
> doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B 
> == A)
> and I honestly feel that it goes beyond nitpicking because of that. 
> 
> It's also a fallacy that there will always be an observed value and an
> expected value. For example:
> 
>   self.assertEqual(method_a(), method_b())
> 
> Which one is observed and which one is expected? I think this proposal is just
> reading into the parameter names a bit too much.

If you are using assertEqual with 2 variable values... you are doing
your test wrong.

I was originally in your camp. But honestly, the error message provided
to the user does say expected and observed, and teaching everyone that
you have to ignore the error message is a much harder thing to do than
flip the code to conform to it, creating less confusion.

-Sean

-- 
Sean Dague
http://dague.net



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Matthew Treinish]

On Fri, Nov 21, 2014 at 10:30:59AM -0800, Joe Gordon wrote:
> On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui <
> sahid.ferdja...@redhat.com> wrote:
> 
> > On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> > > On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> > > sahid.ferdja...@redhat.com> wrote:
> > >
> > > > This is something we can call nitpiking or low priority.
> > > >
> > >
> > > This all seems like nitpicking for very little value. I think there are
> > > better things we can be focusing on instead of thinking of new ways to
> > nit
> > > pick. So I am -1 on all of these.
> >
> > Yes as written this is low priority but something necessary for a
> > project like Nova it is.
> >
> >
> Why do you think this is necessary?
> 
> 
> > Considered that I feel sad to take your time. Can I suggest you to
> > take no notice of this and let's others developers working on Nova too
> > do this job ?
> >
> >
> As the maintainer of openstack-dev/hacking and as a nova core, I don't
> think this is worth doing at all. Nova already has enough on its plate and
> doesn't need extra code to review.
> 

I tend to agree, also hacking rules are there to reduce the mental load of
reviewers so that they have less to worry about doing a review. Honestly, none
of these proposals seem like anything a reviewer should ever worry about in the
first place, let alone a reason to -1 a patch. (either automatically with a rule
or manually)


pgp89v3VuxZ0N.pgp
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Matthew Treinish]

On Fri, Nov 21, 2014 at 07:15:49PM +0100, jordan pittier wrote:
> Hey,
> I am not a Nova developer but I still have an opinion.
> 
> >Using boolean assertions
> I like what you propose. We should use and enforce the assert* that best 
> matches the intention. It's about semantic and the more precise we are, the 
> better.
> 
> >Using same order of arguments in equality assertions
> Why not. But I don't know how we can write a Hacking rule for this. So you 
> may fix all the occurrences for this now, but it might get back in the future.

Ok I'll bite, besides the enforceability issue which you pointed out, it just
doesn't make any sense, you're asserting 2 things are equal: (A == B) == (B == 
A)
and I honestly feel that it goes beyond nitpicking because of that. 

It's also a fallacy that there will always be an observed value and an
expected value. For example:

  self.assertEqual(method_a(), method_b())

Which one is observed and which one is expected? I think this proposal is just
reading into the parameter names a bit too much.


> 
> >Using LOG.warn instead of LOG.warning
> I am -1 on this. The part that comes after LOG. (LOG.warning, LOG.error, 
> LOG.debug, etc) is the log level, it's not a verb. In syslog, the well-known 
> log level is "warning" so the correct method to use here is, imo, 
> log.warning().
> 
> Have you concidered submitting this hacking rules to the hacking project here 
> : https://github.com/openstack-dev/hacking ? I am sure these new rules makes 
> sense on other openstack projects.
> 
> Jordan  
> 
> - Original Message -
> From: "Sahid Orentino Ferdjaoui" 
> To: "OpenStack Development Mailing List (not for usage questions)" 
> 
> Sent: Friday, November 21, 2014 5:57:14 PM
> Subject: Re: [openstack-dev] [nova] Proposal new hacking rules
> 
> On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> > On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> > sahid.ferdja...@redhat.com> wrote:
> > 
> > > This is something we can call nitpiking or low priority.
> > >
> > 
> > This all seems like nitpicking for very little value. I think there are
> > better things we can be focusing on instead of thinking of new ways to nit
> > pick. So I am -1 on all of these.
> 
> Yes as written this is low priority but something necessary for a
> project like Nova it is.
> 
> Considered that I feel sad to take your time. Can I suggest you to
> take no notice of this and let's others developers working on Nova too
> do this job ?
> 



pgpclj1xSMjtI.pgp
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Joe Gordon]

On Fri, Nov 21, 2014 at 8:57 AM, Sahid Orentino Ferdjaoui <
sahid.ferdja...@redhat.com> wrote:

> On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> > On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> > sahid.ferdja...@redhat.com> wrote:
> >
> > > This is something we can call nitpiking or low priority.
> > >
> >
> > This all seems like nitpicking for very little value. I think there are
> > better things we can be focusing on instead of thinking of new ways to
> nit
> > pick. So I am -1 on all of these.
>
> Yes as written this is low priority but something necessary for a
> project like Nova it is.
>
>
Why do you think this is necessary?


> Considered that I feel sad to take your time. Can I suggest you to
> take no notice of this and let's others developers working on Nova too
> do this job ?
>
>
As the maintainer of openstack-dev/hacking and as a nova core, I don't
think this is worth doing at all. Nova already has enough on its plate and
doesn't need extra code to review.


> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[jordan pittier]

Hey,
I am not a Nova developer but I still have an opinion.

>Using boolean assertions
I like what you propose. We should use and enforce the assert* that best 
matches the intention. It's about semantic and the more precise we are, the 
better.

>Using same order of arguments in equality assertions
Why not. But I don't know how we can write a Hacking rule for this. So you may 
fix all the occurrences for this now, but it might get back in the future.

>Using LOG.warn instead of LOG.warning
I am -1 on this. The part that comes after LOG. (LOG.warning, LOG.error, 
LOG.debug, etc) is the log level, it's not a verb. In syslog, the well-known 
log level is "warning" so the correct method to use here is, imo, log.warning().

Have you concidered submitting this hacking rules to the hacking project here : 
https://github.com/openstack-dev/hacking ? I am sure these new rules makes 
sense on other openstack projects.

Jordan  

- Original Message -
From: "Sahid Orentino Ferdjaoui" 
To: "OpenStack Development Mailing List (not for usage questions)" 

Sent: Friday, November 21, 2014 5:57:14 PM
Subject: Re: [openstack-dev] [nova] Proposal new hacking rules

On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> sahid.ferdja...@redhat.com> wrote:
> 
> > This is something we can call nitpiking or low priority.
> >
> 
> This all seems like nitpicking for very little value. I think there are
> better things we can be focusing on instead of thinking of new ways to nit
> pick. So I am -1 on all of these.

Yes as written this is low priority but something necessary for a
project like Nova it is.

Considered that I feel sad to take your time. Can I suggest you to
take no notice of this and let's others developers working on Nova too
do this job ?

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Sahid Orentino Ferdjaoui]

On Thu, Nov 20, 2014 at 02:00:11PM -0800, Joe Gordon wrote:
> On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
> sahid.ferdja...@redhat.com> wrote:
> 
> > This is something we can call nitpiking or low priority.
> >
> 
> This all seems like nitpicking for very little value. I think there are
> better things we can be focusing on instead of thinking of new ways to nit
> pick. So I am -1 on all of these.

Yes as written this is low priority but something necessary for a
project like Nova it is.

Considered that I feel sad to take your time. Can I suggest you to
take no notice of this and let's others developers working on Nova too
do this job ?

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] Proposal new hacking rules

[Joe Gordon]

On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <
sahid.ferdja...@redhat.com> wrote:

> This is something we can call nitpiking or low priority.
>

This all seems like nitpicking for very little value. I think there are
better things we can be focusing on instead of thinking of new ways to nit
pick. So I am -1 on all of these.


>
> I would like we introduce 3 new hacking rules to enforce the cohesion
> and consistency in the base code.
>
>
> Using boolean assertions
> 
>
> Some tests are written with equality assertions to validate boolean
> conditions which is something not clean:
>
>   assertFalse([]) asserts an empty list
>   assertEqual(False, []) asserts an empty list is equal to the boolean
>   value False which is something not correct.
>
> Some changes has been started here but still needs to be appreciated
> by community:
>
>  * https://review.openstack.org/#/c/133441/
>  * https://review.openstack.org/#/c/119366/
>
>
> Using same order of arguments in equality assertions
> 
>
> Most of the code is written with assertEqual(Expected, Observed) but
> some part are still using the opposite. Even if they provide any real
> optimisation using the same convention help reviewing and keep a
> better consistency in the code.
>
>   assertEqual(Expected, Observed) OK
>   assertEqual(Observed, Expected) KO
>
> A change has been started here but still needs to be appreciated by
> community:
>
>  * https://review.openstack.org/#/c/119366/
>
>
> Using LOG.warn instead of LOG.warning
> -
>
> We can see many time reviewers -1ed a patch to ask developer to use
> 'warn' instead of 'warning'. This will provide no optimisation
> but let's finally have something clear about what we have to use.
>
>   LOG.warning: 74
>   LOG.warn:319
>
> We probably want to use 'warn'
>
> Nothing has been started from what I know.
>
>
> Thanks,
> s.
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev