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

[Ivan Necas]

----- 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
>