Rails is now a conditional option #36

javierg · 2018-03-16T19:29:41Z

This PR adds checks for Rails definition in order to use the gem without it.

for this to work, there is the need to load the tasks manually in the Rakefile of the ruby project.
We should research on doing this a priori

current implementation sample Rakefile is like:

require 'bundler/setup'
require 'rake'
Bundler.require

# Load Biran Gem tasks
config = Gem::Specification.find_by_name('biran')
Dir["#{config.gem_dir}/lib/tasks/*.rake"].each do |file|
  Rake::load_rakefile(file)
end

$LOAD_PATH.unshift(File.expand_path('../lib', __FILE__))
Dir.glob('lib/tasks/*.rake').each {|r| import r}

Also since we have by default creating database.yml this PR is a transition in this part as it will still create that by default. But if you add db_config: false to the app_config.yml file, under defaults > app the file and the task will be skipped

brlanier · 2018-03-16T19:34:11Z

I have read that using Bundler.require is "bad" because you are loading a bunch of stuff you don't specifically need. It loads all the things from all the gems in LOAD_PATH. We should look at being able to support something as simple as
require 'biran/biran_tasks'
or something like that. I have seen that doing the gem spec thing is kind of common, so not opposed to that, but would really love a way were we only have to require one thing and have the gem do what it needs for us.

javierg · 2018-03-16T19:37:04Z

I have read that using Bundler.require is "bad" because you are loading a bunch of stuff you don't specifically need. It loads all the things from all the gems in LOAD_PATH.

where did you read this?

javierg · 2018-03-16T19:49:04Z

http://myronmars.to/n/dev-blog/2012/12/5-reasons-to-avoid-bundler-require

VS

https://anti-pattern.com/bundler-setup-vs-bundler-require

The argument that you dont want to require all gems in Gemefile (not loadpath) is not that strong, since this is what you expect when using a Gemfile, if not, there is no use for bundler.

Also, rails will have many more Gems that any simple ruby project we have, and rails do fine using Bundler.require

brlanier

Overall not a fan of the changes. I can't tell what is actually needed to solve the ruby vs rails issue and what is being used to change another behavior. I will pull this down and test, but I have questions in the comments

brlanier · 2018-03-16T19:38:44Z

lib/biran/config_defaults.rb

@@ -54,6 +54,9 @@ def secrets_file
    end

    def default_db_config_file
+      unless defined? Rails
+        return "#{app_defaults_init[:app][:base_path]}/#{configuration.config_dirname}/#{configuration.db_config_filename}"
+      end


Isn't this already handled in the configuration object?

def base_path return @base_path if @base_path @base_path = Rails.root if defined? Rails @base_path ||= './' end

Do we need the unless block? Can we just use the return string and drop the Rails.root line below?

Is that no we don't need the block or is it no we can't use the return string.
Also, I think this would be better anyway

def default_db_config_file File.join(config_dir, configuration.db_config_filename) end

But that is untested.

Let me check this one, probably not needed.

I believe the File.join line I have above works as expected. I have done some basic test against another app that uses rails and the database.yml file with my proposed set of minimal changes and it seems to work as expected.

Is not working as expected. Once I remove this, running the task raises the exception NameError: uninitialized constant Biran::ConfigDefaults::Rails

brlanier · 2018-03-16T19:39:09Z

lib/biran/config.rb

+    end
+
+    def db_file_to_generate
+      { database: { extension: '.yml' } }


I don't understand this change. Why separate out the db file part?

Because if we are removing the rails dependency is no longer a default file to generate. This PR still generate it by default, but having it in the default list of files will not allow it to filter it out easily.

There is no need to generate vhost by default either. I picked sensible defaults for the most of our apps. It gets overridden by the values in an initializer or config/app_config.yml. So not sure what you mean by not filtered out easily. We have used biran on many projects that don't need the database.yml file.
If your concern is removing something that is rails centric from the default values, then I'm okay with simply removing the file from the default list. But I don't see the need for all the extra logic on what should be some simple generic defaults. Plus, this change should have been in its own PR as I don't see how it affects the central problem of the gem not loading in a non rails app.

Ok, I understand your concern, but this fails without a Rails.root existing. It might be a different issue, but this will never work without fixing this so it is coupled

Its not needed to solve the problem. Removing the Rails.root is needed. You only simply need to remove the database line from the files to generate method returning the default options and fix the default path using Rails.root to satisfy your concerns about being too rails like.

well, if I leave this file in there, Biran stops working ... so it is not working

brlanier · 2018-03-16T19:46:54Z

lib/biran/configurinator.rb

@@ -30,12 +30,12 @@ def initialize
    end

    def file_tasks
-      files_to_generate.keys
+      config_files_to_generate.keys


We spend a lot of time on the naming of this part... I don't understand the name change. This is going to be used for not only config files, but other files outside of config and maybe not used for config at all? The vhost file is not really a config file, however, it could be argued either way.

this is a config Gem, every file to be generated is a config file... vhost is not config? it has conf in the extension... so not sure if we making this very narrow distinctions help us

As stated, we went round and round in the original PR over naming of this. There is no need to add "config" to the beginning of a method in a config object. Its redundant. My argument is that the gem is used to help configure your app. Maybe that is creating config files, maybe that is generating files needed for the app to work properly that aren't really configuring anything. Its a context thing. I just don't like the naming and I still think its an unnecessary change since all it appears to be doing is wrapping a default value that is more easily removed as a default value.

This is a delegated method, so it is a good pattern, it is the files to generate from the. Config object

brlanier · 2018-03-16T19:51:36Z

lib/biran/configurinator.rb

+    def config_files_to_generate
+      files = configuration.files_to_generate
+      return files unless default_db_file?
+      files.merge configuration.db_file_to_generate


I still don't understand the separation here of treating the db file as different. This appears to all be just guarding against the idea of a missing db_config.yml file. The original defaults of generating both the vhost and the database.yml file was based on the idea that for the majority of our projects, that is what we want, so no additional config would be needed. Why change the defaults of a gem based on the existence of a file. Just because I have a db_config.yml file doesn't necessarily mean I want the database.yml file generated. But without changing anything in app_config, it appears this would happen based on presence of file.
So my question is, what problem is this change trying to solve?

it is solving preventing Biran trying to generate the database.yml file and looking for _database.yaml.erb the presence of a file is only a condition, but there is some other checks that doesnt have to do with db_config there is actually no need for a db_config.yml file not sure if this is technical debt or what. But I still need to take out database out from default tasks and files to be generated.

This is more easily solved by simply removing the database file from the default hash of files to be generated. Biran won't try to generate the file IF the config is set in app_config.yml. I still say this has nothing to do with preventing loading in ruby only.

Tried that a lot. It seems to be be very coupled to the code so there is no way this will work ...

This is more easily solved by simply removing the database file from the default hash of files to be generated.

yeah that fixes the issue, but makes it incompatible with all our current installations

brlanier · 2018-03-16T19:54:07Z

lib/biran/configurinator.rb

@@ -121,7 +131,7 @@ def sanitize_config_files files_list
      lambda do |file, _|
        files_list[file] ||=  {extension: ''}
        ext = files_list[file].fetch(:extension, '').strip
-        ext.prepend('.') unless ext.starts_with?('.') || ext.empty?
+        ext.prepend('.') unless ext.start_with?('.') || ext.empty?


So this is a ruby vs rails change, but don't we get starts_with? because of ActiveSupport?

yes, so that is why the activesupport requirement added... also, is start_with? there was a typo in here

Its not a typo. It was working. It looks to me more like the rails version is using the ruby version now anyway. But not a typo.

Probably, I might need to require another rails dependency then

But if there is no difference in the output between the core rails method and Ruby one I prefer the Ruby core one.

brlanier

Still testing, but I still have comments about unnecessary changes and I am not sure which change is supposed to fix the problem of loading in ruby

brlanier · 2018-03-16T20:15:53Z

lib/biran/config.rb

+    end
+
+    def db_file_to_generate
+      { database: { extension: '.yml' } }


There is no need to generate vhost by default either. I picked sensible defaults for the most of our apps. It gets overridden by the values in an initializer or config/app_config.yml. So not sure what you mean by not filtered out easily. We have used biran on many projects that don't need the database.yml file.
If your concern is removing something that is rails centric from the default values, then I'm okay with simply removing the file from the default list. But I don't see the need for all the extra logic on what should be some simple generic defaults. Plus, this change should have been in its own PR as I don't see how it affects the central problem of the gem not loading in a non rails app.

brlanier · 2018-03-16T20:17:15Z

lib/biran/config_defaults.rb

@@ -54,6 +54,9 @@ def secrets_file
    end

    def default_db_config_file
+      unless defined? Rails
+        return "#{app_defaults_init[:app][:base_path]}/#{configuration.config_dirname}/#{configuration.db_config_filename}"
+      end


Is that no we don't need the block or is it no we can't use the return string.
Also, I think this would be better anyway

def default_db_config_file File.join(config_dir, configuration.db_config_filename) end

But that is untested.

brlanier · 2018-03-16T20:20:47Z

lib/biran/configurinator.rb

@@ -30,12 +30,12 @@ def initialize
    end

    def file_tasks
-      files_to_generate.keys
+      config_files_to_generate.keys


As stated, we went round and round in the original PR over naming of this. There is no need to add "config" to the beginning of a method in a config object. Its redundant. My argument is that the gem is used to help configure your app. Maybe that is creating config files, maybe that is generating files needed for the app to work properly that aren't really configuring anything. Its a context thing. I just don't like the naming and I still think its an unnecessary change since all it appears to be doing is wrapping a default value that is more easily removed as a default value.

brlanier · 2018-03-16T20:23:04Z

lib/biran/configurinator.rb

+    def config_files_to_generate
+      files = configuration.files_to_generate
+      return files unless default_db_file?
+      files.merge configuration.db_file_to_generate


This is more easily solved by simply removing the database file from the default hash of files to be generated. Biran won't try to generate the file IF the config is set in app_config.yml. I still say this has nothing to do with preventing loading in ruby only.

brlanier · 2018-03-16T20:24:04Z

lib/biran/configurinator.rb

@@ -121,7 +131,7 @@ def sanitize_config_files files_list
      lambda do |file, _|
        files_list[file] ||=  {extension: ''}
        ext = files_list[file].fetch(:extension, '').strip
-        ext.prepend('.') unless ext.starts_with?('.') || ext.empty?
+        ext.prepend('.') unless ext.start_with?('.') || ext.empty?


Its not a typo. It was working. It looks to me more like the rails version is using the ruby version now anyway. But not a typo.

brlanier · 2018-03-16T20:34:37Z

The only changes needed to work with ruby were the following:

Adding active support as dependency
Changes to include yaml, erb, and active support in lib/biran.rb
Changing the Rails.root reference in the default db_config_file method
The starts_with to start_with change

I haven't tested whether File.join(config_dir, configuration.db_config_filename) works as a replacement for the default config file

brlanier · 2018-03-16T21:05:29Z

Based on your work, this is all that was needed for me to get this working in a ruby app and a rails app. - #37

javierg · 2018-03-16T21:07:17Z

Based on your work, this is all that was needed for me to get this working in a ruby app and a rails app. - #37

I was trying to make this version compatible with our current base installation.

Also, not sure how will tell Biran to process database yml now

can you post a sample?

javierg · 2018-03-16T21:10:18Z

moving conversation to #37

javierg and others added 3 commits March 16, 2018 13:21

rails is now a conditional option

b740a9c

add dependencies

12da328

Merge branch 'master' into make_rails_conditional

b34ea4a

brlanier requested changes Mar 16, 2018

View reviewed changes

brlanier reviewed Mar 16, 2018

View reviewed changes

javierg closed this Mar 16, 2018

brlanier mentioned this pull request Mar 16, 2018

Remove Rails dependencies so gem will work in ruby apps #37

Merged

Rails is now a conditional option #36

Rails is now a conditional option #36

Conversation

javierg commented Mar 16, 2018 • edited Loading

brlanier commented Mar 16, 2018

javierg commented Mar 16, 2018

javierg commented Mar 16, 2018

brlanier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brlanier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brlanier commented Mar 16, 2018

brlanier commented Mar 16, 2018

javierg commented Mar 16, 2018

javierg commented Mar 16, 2018

javierg commented Mar 16, 2018 •

edited

Loading