On Tue, Jun 23, 2015 at 3:59 PM, Mads Kiilerich <[email protected]> wrote: > On 06/22/2015 10:09 PM, Thomas De Schampheleire wrote: >> >> On Mon, Jun 22, 2015 at 7:05 PM, Mads Kiilerich <[email protected]> >> wrote: >>> >>> On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote: >>>> >>>> # HG changeset patch >>>> # User Thomas De Schampheleire <[email protected]> >>>> # 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 _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
