[katello-devel] A couple of recent changes in the codebase

Fri Feb 1 15:22:52 UTC 2013

My two cents:

All validators in a central, easy to find location.
Validators apply to the application, and specifically the model.  So, +1 to
either

app/validators (my preference)

or

app/models/validators

- Eric

On Fri, Feb 1, 2013 at 10:12 AM, Ivan Necas <inecas at redhat.com> wrote:

>
>
> ----- Original Message -----
> > On Friday 01 of February 2013 12:53:09 Dmitri Dolguikh wrote:
> > > On 01/02/13 12:16 PM, Marek Hulan wrote:
> > > >> - A large number of validators have been moved into
> > > >> lib/validators. It
> > > >> could be argued that a few of those are not Katello-specific,
> > > >> but the
> > > >> bulk of them are. Any code that describes peculiarities of
> > > >> Katello
> > > >> domain belongs to app/model. Unless you are writing something
> > > >> non
> > > >> Katello-specific (ask yourself if the code could be packaged as
> > > >> a gem),
> > > >> it does not belong in lib/.
> > > >
> > > > Hi,
> > > >
> > > > this validators moving to one place was my commit. I see your
> > > > point and I
> > > > agree that some validators are Katello specific so lib/ is not
> > > > 100% right
> > > > place for them. However my main motivation was to get all
> > > > validators to
> > > > one place. Validators are not models so I think they should not
> > > > be in
> > > > app/models. Maybe we could move them to app/validators or if
> > > > you'd like
> > > > app/models/validators but second option would conflict with
> > > > namespacing
> > > > conventions... Also separating some validators to lib/ and some
> > > > to app/
> > > > seems to me like a useless obstacle for the future. No one wants
> > > > to think
> > > > about whether this validator is Katello specific or not.
> > > >
> > > > So to make long story short, it's not so important for me where
> > > > validators
> > > > will be placed but I'd really like to have them all in one
> > > > directory.
> > > >
> > > > Comments anyone?
> > >
> > > The model isn't something just entities that can be persisted in a
> > > database - the model is anything that contains "business-logic".
> > > That
> > > includes validators, services (things that perform operations on
> > > several
> > > model classes), etc. These all belong to models.
> > >
> > > Now, the source layout isn't as important in dynamic languages as
> > > it is
> > > with compiled languages, since we are not dealing with compilation
> > > units
> > > and are not trying to minimize compilation/build time. The source
> > > layout, however, can be used to simplify the discovery of model
> > > relations and functionality by borrowing projects using languages
> > > like
> > > java or C++: move the code closer to where it's being used and
> > > minimize
> > > the dependencies on the module.
> > >
> > > Here's my proposal:
> > >   - move shared, Katello-specific validators into
> > >   app/models/validators
> > >   - move unique (used by one model class) validators either in the
> > >   model
> > > classes themselves, or into correspondingly named modules in
> > > app/models/.
> > >   - leave non Katello-specific validators in lib/
>
> Here's another point of view (that might bring some light as well):
>
> I started working on making our API documentation more DRYish. One
> of the things I would like to achieve is to get the information from
> validators (that bear very interesing documentation value). Few
> related findings:
>
> * We have duplicities in validations, compare:
>
>       ActivationKey#environment_not_library [1]
>
>    and
>
>       Changeset: validates_with Validators::NotInLibraryValidator [2]
>
>
> * We mix validating with method [1] vs. validating with class [2]. This
> might be worth
>   unifying
>
> * From the documentations perspecitve, there's no way you can extract
> anything from
>   the validations defined by method. In the [2] case, the validator
> appears in the
>   `Changeset.validators` and we can work with it further - The Apipie part
> is almost ready
>   for that.
>
> * If we extract the method validators into classes, its 15 more classes in
> the lib/validators
>   directory: might get messy, especially when some validators really are
> just applicable
>   for one class (such as Provider#constraint_redhat_update). Having them
> separated in a module
>   sounds reasonable, agreeing with dmitri
>
> * Having the validators in the `app/models` with all the models is quite
> messy too, and I agree
>   that it might lead in duplicate definitions, just becuase one doesn't
> notice something to
>   be used is already there. I don't
>   have much preferrences about `lib` vs `app/validators` vs
> `app/models/validators`.
>
>
> [1] -
> https://github.com/Katello/katello/blob/master/src/app/models/activation_key.rb#L64
> [2] -
> https://github.com/Katello/katello/blob/master/src/app/models/changeset.rb#L43
>
>
> >
> > I don't agree. Benefits of having validators in one place I see:
> > + developers can see whether there is some existing/similar validator
> > already,
> > thus leads to more DRY code
> > + developers don't have to think whether some validator should be
> > considered
> > non Katello-specific (honestly - no one will ever use 10 lines in
> > form of our
> > validator in another application)
> > + developers know where to look at
> >
> > And I don't see any real cons. I think we should not accept rules
> > just to be
> > 100% consistently organized without other benefits. I think we should
> > concentrate more on simplicity. The more rules we'll try to enforce
> > the harder
> > we'll be able to add new code. Let's try to keep it stupid simple -
> > it's
> > validator, move it into validators directory.
> >
> > --
> > Marek
> >
> > _______________________________________________
> > katello-devel mailing list
> > katello-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/katello-devel
> >
>
> _______________________________________________
> katello-devel mailing list
> katello-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/katello-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/katello-devel/attachments/20130201/ae0adb48/attachment.htm>