[PATCH 1 of 2] model: check for invalid users centrally

[Thomas De Schampheleire]

On Tue, Jun 23, 2015 at 3:59 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 06/22/2015 10:09 PM, Thomas De Schampheleire wrote:
>>
>> On Mon, Jun 22, 2015 at 7:05 PM, Mads Kiilerich <mads at kiilerich.com>
>> wrote:
>>>
>>> On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>>> # Date 1434984724 -7200
>>>> #      Mon Jun 22 16:52:04 2015 +0200
>>>> # Node ID 68032f8bd7f52c676bb5143a03b0dfd5df242f0c
>>>> # Parent  7303d0035d82a20f93df1c78b3358403b7ff9827
>>>> model: check for invalid users centrally
>>>>
>>>> Commit 9a23b444a7fefe5b67e57a42d26343787d992874 introduced
>>>> UserInvalidException. The raising of this exception, however, was done
>>>> only
>>>> in pullrequest context, while it could have been done in
>>>> BaseModel._get_user().
>>>
>>>
>>> Why not be more consistent and do it in _get_instance (using a more
>>> generic
>>> EntityNotFound exception (or something like that)? Is that the end goal
>>> and
>>> this just an intermediate step?
>>
>> Would you translate the EntityNotFound in some other more specific
>> exception class, for example from _get_user, or would EntityNotFound
>> be the final exception?
>
>
> I think that would be fine.
>
> In some cases, there will be multiple users involved anyway and it will not
> be possible to distinguish _which_ of them failed. The description should of
> course explain what it was that failed.
>
>> This user part is an evolution from my initial introduction of
>> UserInvalidException, but as I hinted below I think it makes sense to
>> expand it. Doing it directly in get_instance is indeed more logical, I
>> can do that at the same time.
>
>
> Ok - so you will rework this patch to do that?

Yes, I will...

/Thomas