Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Railtie and its requires #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ribose-jeffreylau
Copy link
Contributor

No description provided.

@skalee
Copy link
Contributor

skalee commented Mar 13, 2019

This pull request makes me realize that there are literally no tests which cover railtie… I'm opening a new issue #56 about that.

require "rails"

module ActiveUUID
class Railtie < Rails::Railtie
railtie_name :activeuuid

config.to_prepare do
ActiveUUID::Patches.apply!
ActiveUUID::ConnectionPatches.apply!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This very invocation concludes active_uuid/connection_patches.rb, so I suppose moving line no. 2 here is a better option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By line no. 2, is that require "rails or ActiveUUID::ConnectionPatches.apply! or some other lines?

Where does the line need to be moved?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have amended that commit. See: c4ffad2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevertheless, I'm not sure if we need Railtie at all. I don't like that it applies patches unconditionally. See my other comment: #55 (comment).

@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #55 into master will decrease coverage by 0.19%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #55     +/-   ##
=========================================
- Coverage   99.05%   98.86%   -0.2%     
=========================================
  Files          21       23      +2     
  Lines         425      439     +14     
=========================================
+ Hits          421      434     +13     
- Misses          4        5      +1
Impacted Files Coverage Δ
spec/integration/railtie_spec.rb 100% <100%> (ø)
lib/active_uuid/railtie.rb 85.71% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd536eb...1cb2cd3. Read the comment docs.

@skalee
Copy link
Contributor

skalee commented Mar 14, 2019

One thing I've changed since original ActiveUUID is that monkey patching is now facultative (see README):

No core feature relies on Rails monkey patching. Monkey patches can interfere with other gems, and lead to issues. Nevertheless, some convenient features (currently, migration methods only) are provided via monkey patching. Enabling them is entirely optional, and their absence can be workarounded easily.

This pull request violates it, as loading the Railtie applies monkey patches unconditionally.

I don't know… maybe we should remove this Railtie entirely? Or add some configuration option? Thoughts, @ribose-jeffreylau? I'll probably do some research.

@ribose-jeffreylau
Copy link
Contributor Author

@skalee We see the following options:

  • Add a check for the constant Rails before defining this Railtie;
  • Use some other configuration methods instead of Railtie (i.e. remove Railtie);

We are OK either way. There may be other ways but these two are the most obvious that I can think of right now.

@skalee
Copy link
Contributor

skalee commented Mar 20, 2019

@ribose-jeffreylau Will following satisfy your needs?

  1. Railtie removal
  2. Writing something like gem "activeuuid", require: "active_uuid/all" in Gemfile

If the only purpose of this Railtie is to apply connection patches, then I'd rather like to remove it entirely, and encourage users to enable them explicitly if they want them.

@ronaldtse
Copy link
Contributor

Writing something like gem "activeuuid", require: "active_uuid/all" in Gemfile

This works in Gemfile but not with gemspec inheritance. As long as we have documentation to indicate the user needs to write require "active_uuid/all", it should be good enough.

Or would something like this work?

if defined?(Rails)
  require "activeuuid/rails"
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants