On 23.10.2015 13:53, Martin Basti wrote:
>
>
> On 23.10.2015 13:53, Tomas Babej wrote:
>>
>> On 10/23/2015 01:51 PM, Martin Basti wrote:
>>>
>>> On 23.10.2015 13:49, Tomas Babej wrote:
>>>> On 10/23/2015 12:49 PM, Martin Basti wrote:
>>>>> On 23.10.2015 09:34, Martin Basti wrote:
>>>>>> On 23.10.2015 09:31, Tomas Babej wrote:
>>>>>>> On 10/22/2015 05:49 PM, Simo Sorce wrote:
>>>>>>>> On 22/10/15 11:29, Martin Basti wrote:
>>>>>>>>> Hello all,
>>>>>>>>>
>>>>>>>>> in current master branch we have mixed usage of literals 0, 1 and
>>>>>>>>> constants MIN_DOMAIN_LEVEL, MAX_DOMAIN_LEVEL, and it is quite mess.
>>>>>>>>>
>>>>>>>>> I suggest to use names for domain levels:
>>>>>>>>>
>>>>>>>>> COMPAT_DOMAIN_LEVEL = 0
>>>>>>>>> PROMOTION_DOMAIN_LEVEL = 1
>>>>>>>>> UBER_NEW_FEATURE_DOMAIN_LEVEL = 2
>>>>>>>>>
>>>>>>>>> MIN_DOMAIN_LEVEL = COMPAT_DOMAIN_LEVEL (=0)
>>>>>>>>> MAX_DOMAIN_LEVEL = UBER_NEW_FEATURE_DOMAIN_LEVEL (=2)
>>>>>>>>>
>>>>>>>>> Benefits:
>>>>>>>>> * ability to grep it in code
>>>>>>>> Call them DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1
>>>>>>>>
>>>>>>>>> * better readability in code
>>>>>>>> Sure, but random names are not appropriate imo
>>>>>>> I'm with you guys on this, it's a good idea. Let's go with the
>>>>>>> DOMAIN_LEVEL_X naming though, it will be probably easier to remember.
>>>>>>>
>>>>>>> One thing to add to the discussion - MIN/MAX_DOMAIN_LEVEL denotes only
>>>>>>> the minimal/maximal domain level supported by the given IPA server,
>>>>>>> not
>>>>>>> the minimal/maximal domain level ever shipped by FreeIPA project.
>>>>>>>
>>>>>>> Currently, those two coincide, but in general they might be
>>>>>>> different if
>>>>>>> we ever raise the minimal level a decide to deprecate, say, domain
>>>>>>> level
>>>>>>> 0 or 1. It's a subtle but important difference.
>>>>>>>
>>>>>>> Tomas
>>>>>> Thank you all for your opinion,
>>>>>>
>>>>>> I will implement DOMAIN_LEVEL_X constants and send patch.
>>>>>>
>>>>>> Thanks.
>>>>>> Martin^2
>>>>>>
>>>>> Patch attached.
>>>>> O hope, I did not miss anything hardcoded.
>>>> I think we can safely hardcode domain levels as numbers in the error
>>>> messages. Substituting {dl} for constants.DOMAIN_LEVEL_0 in the error
>>>> message makes little sense to me, as the value of
>>>> constants.DOMAIN_LEVEL_0 will not ever change.
>>> However, with substituting is easier to find occurrences of domain
>>> levels in comments and messages in case of refactoring.
>> In case of refactoring of what? All the error messages containing
>> reference to a domain level 0? :)
> Exactly! :)
Tomas, that one day we decide to drop support for domain level 0. We will have
to hunt down all references to it and using constant for help messages etc.
might be handy because it will show up in results of grep, so we do not forget
to wipe domain level 0 from texts.
Nitpick just to make you feel that I read the patch (does not warrant a NACK):
I would rather see conditions in form of (<= or >=) instead of (< or >)
because now you have to grep for domain level +- 1 to get all references to
particular domain level :-D
Nevermind, this is just a nitpick.
--
Petr^2 Spacek
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
