Include modules instead of reopening classes.

[jrafanie]

Related to #14128

We should either use RssFeed.class_eval to reopen the class to force
rails to load the RssFeed model first or the technique in this commit,
which is to just include the module in RssFeed. Note, this module was
already being included so there was no reason to reopen the class also.

The existing layout of re-opening the RssFeed with different class
definitions:

rss_feed/import_export.rb:
class RssFeed

rss_feed.rb:
class RssFeed < ApplicationRecord

can cause TypeError: superclass mismatch for class RssFeed if
the import_export.rb is loaded first.

[jrafanie]

@isimluk @lpichler Please review.

[lpichler]

It is already on line 11

[jrafanie]

OMG, WAT, thanks!

[jrafanie]

Why are we reopening the class if we're already including the module???

[jrafanie]

Thanks @lpichler. I guess I didn't even expect that this module was already included... I moved the include with the other include so it's more obvious.

👍

[lpichler]

just one thing I found, see comment

[lpichler]

@jrafanie @isimluk, as we noticed, for this extending classes we have two patterns in our application as was described, for example taken from this PR:

1. Extending by redefinition of class

class RssFeed
  module ImportExport
    extend ActiveSupport::Concern

2. Extending by module

module RssFeed::ImportExport
  extend ActiveSupport::Concern
  ...

with include_concern in 'main' class

class RssFeed
  include_concern 'ImportExport'

For me is sort of confusing to have statement of class definition on two places but maybe it is ok in ruby.

or should we rather use only 2) ? What do you think ?

[isimluk]

I don't have a strong opinion over the change. I felt re-open is just fine (ruby is strict and fails early).

except for Libor's comment I am 👍 🍰 🥄

[miq-bot]

Checked commit jrafanie@0229960 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 1 offense detected

app/models/rss_feed/import_export.rb

• ❗ - Line 6, Col 5 - Style/WordArray - Use %w or %W for an array of words.

[jrafanie]

I don't have a strong opinion over the change. I felt re-open is just fine (ruby is strict and fails early).

@isimluk If this is was straight ruby where you require your dependencies and don't rely on rails autoload, I'd totally agree. With rails autoload, if the constant is already defined, rails won't autoload rss_feed.rb if import_export.rb already defined RssFeed constant. I think the only options are RssFeed.class_eval in the extension to force rails to autoload RssFeed model first or use modules like this PR does.

Ok, I made the change recommended by @lpichler and updated the description. Thanks again for the 👀

[isimluk]

With rails autoload, if the constant is already defined, rails won't autoload rss_feed.rb if import_export.rb
already defined RssFeed constant.

I have looked into the rails autoload code last week (in relation with #14128). I haven't found a place/condition where this could happen silently. If the developer makes a mistake (hardoces require), he/she will soon learn about the mistake. (Unless of course, they do not silence_warnings). Hence, I am not terrified by re-open now.

Historical evidence speaks for itself. This problem was never existed, until we introduced coverall-load-all thingy.

So we can get a rid of this pattern (partially). However, I would be much more concerned about the silence_warnings.

[jrafanie]

Historical evidence speaks for itself. This problem was never existed, until we introduced coverall-load-all thingy.

@isimluk yeah, this is true for this specific problem. The general problem of autoloading constants in the correct order has hit us many times. Avoiding load order issues by making the base thing include the extending module, what this PR does, and using the RssFeed.class_eval trick are the two ways that seem to get the right behavior. I wish it was easier.