Support multiple db/views paths #237

smdern · 2018-02-14T12:32:44Z

I needed to use this in a rails engine and didn't want to have to copy db_views over for every rails app.

This makes it so the the definition is searched for in multiple paths when trying to call to_sql. Generator paths aren't affected. I leveraged Rails' config.paths for configuring additional db/views paths. I could change it so that the paths are apart of Scenic::Configuration, I just thought putting it in Rails.config.paths['db/views'] would pair nicely with Rails.config.paths['db/migrations'] works.

I'm able to make this work with a rails engine by using the following in engine.rb

    initializer :append_migrations do |app|
      app.config.paths["db/views"] << root.join("db/views")

      unless app.root.to_s.match? root.to_s
        config.paths["db/migrate"].expanded.each do |expanded_path|
          app.config.paths["db/migrate"] << expanded_path
        end
      end
    end

This follows a similar pattern to how migrations are found. It enables Rails engines to define their own db/views. It finds the first entry found in the Rails.application.config.paths["db/views"]. It still assumes Rails.root/db/views when generating a migration.

spec/scenic/definition_spec.rb

coding-chimp · 2018-04-25T07:04:58Z

I pretty much have the same use-case. Would love to see this get merged! 👍

MarcReniu · 2018-06-01T11:03:31Z

Hello!! Anyone know when this PR will be merged? Thanks

derekprior

I'm not familiar with config.paths and there's zero public docs on it. Do you have something you can point me to for this?

Further, are there methods on Definition that we no longer need? (path, full_path)?

lib/scenic/definition.rb

spec/scenic/definition_spec.rb

lib/scenic/definition.rb

calebhearth · 2018-12-05T16:27:21Z

lib/scenic/definition.rb

+    def find_definition
+      definition = views_paths.flat_map do |directory|
+        Dir.glob("#{directory}/**/#{filename}")
+      end.first


Suggested change

end.first

end

It could be a find, but then it would only return the directory that the view exists under. This actually resolves the first matching file that is found under each view_path directory. Plus it could be possibly be nested.

calebhearth · 2018-12-05T16:27:50Z

lib/scenic/definition.rb

+        Dir.glob("#{directory}/**/#{filename}")
+      end.first
+
+      unless definition


This would be more consistent like this:

Suggested change

unless definition

if definition.empty?

calebhearth · 2018-12-05T16:28:20Z

spec/fixtures/db_views/searches_v01.sql

@@ -0,0 +1 @@
+SELECT text 'Hi' as greeting


Suggested change

SELECT text 'Hi' as greeting

SELECT text 'Hi' as greeting

spec/scenic/definition_spec.rb

hound · 2018-12-06T16:09:34Z

spec/scenic/definition_spec.rb

+      Rails.application.config.paths["db/views"] << fixtures_path
+      yield
+    ensure
+      Rails.application.config.paths["db/views"] = original


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:34Z

spec/scenic/definition_spec.rb

+    def with_views_fixtures
+      original = Rails.application.config.paths["db/views"].to_a
+      fixtures_path = File.expand_path("../../fixtures/db_views", __FILE__)
+      Rails.application.config.paths["db/views"] << fixtures_path


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:34Z

spec/scenic/definition_spec.rb

+
+    def with_views_fixtures
+      original = Rails.application.config.paths["db/views"].to_a
+      fixtures_path = File.expand_path("../../fixtures/db_views", __FILE__)


Style/ExpandPathArguments: Use expand_path('../fixtures/db_views', dir) instead of expand_path('../../fixtures/db_views', FILE).
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:35Z

spec/scenic/definition_spec.rb

@@ -52,5 +81,14 @@ module Scenic
        expect(definition.version).to eq "15"
      end
    end
+
+    def with_views_fixtures
+      original = Rails.application.config.paths["db/views"].to_a


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:35Z

spec/scenic/definition_spec.rb

      end

      it "raises an error if the file is empty" do
-        allow(File).to receive(:read).and_return("")
+        definition = Definition.new("empty_view", 1)


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:37Z

spec/scenic/definition_spec.rb

@@ -2,22 +2,51 @@

 module Scenic
  describe Definition do
+    describe "defintion_path" do
+      it "raises an error if file cant be found" do
+        definition = Definition.new("not_valid", 1)


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:38Z

spec/scenic/definition_spec.rb

@@ -2,22 +2,51 @@

 module Scenic
  describe Definition do
+    describe "defintion_path" do
+      it "raises an error if file cant be found" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:38Z

spec/scenic/definition_spec.rb

@@ -2,22 +2,51 @@

 module Scenic
  describe Definition do
+    describe "defintion_path" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:38Z

lib/scenic/railtie.rb

@@ -7,6 +7,8 @@ module Scenic
  # @see Scenic.load
  class Railtie < Rails::Railtie
    initializer "scenic.load" do
+      Rails.application.config.paths.add("db/views")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:39Z

lib/scenic/definition.rb

@@ -28,6 +40,10 @@ def version

    private

+    def views_paths
+      Rails.application.config.paths["db/views"].expanded


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:09:44Z

lib/scenic/definition.rb

+        Dir.glob("#{directory}/**/#{filename}")
+      end.first
+
+      unless definition


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

smdern · 2018-12-06T16:27:01Z

I'm not familiar with config.paths and there's zero public docs on it. Do you have something you can point me to for this?

https://apidock.com/rails/Rails/Engine/Configuration/paths

smdern · 2018-12-06T16:31:37Z

Further, are there methods on Definition that we no longer need? (path, full_path)?

These could probably go away. Currently they're being used in a helper for creating new views using the generator, so maybe they could instead be changed to target_path or something. It looks like path could at least go away since it's just being used by full_path

hound · 2018-12-06T16:54:39Z

spec/scenic/statements_spec.rb

        expect do
          connection.update_view(
            :views,
            version: 1,
-            sql_definition: "a defintion")
+            sql_definition: "a definition")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

hound · 2018-12-06T16:54:39Z

spec/scenic/statements_spec.rb

        expect { connection.update_view :views }.to raise_error(
          ArgumentError,
          /sql_definition or version must be specified/)
      end

-      it "raises an error if both version and sql_defintion are provided" do
+      it "raises an error if both version and sql_definition are provided" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:54:39Z

spec/scenic/statements_spec.rb

@@ -144,18 +144,18 @@ module Scenic
          with(:name, definition.to_sql, no_data: true)
      end

-      it "raises an error if not supplied a version or sql_defintion" do
+      it "raises an error if not supplied a version or sql_definition" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:54:39Z

spec/scenic/statements_spec.rb

@@ -108,7 +108,7 @@ module Scenic
      end

      it "updates a view from a text definition" do
-        sql_definition = "a defintion"
+        sql_definition = "a definition"


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:54:40Z

spec/scenic/statements_spec.rb

        expect do
-          connection.create_view :foo, version: 1, sql_definition: "a defintion"
+          connection.create_view :foo, version: 1, sql_definition: "a definition"


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Metrics/LineLength: Line is too long. [81/80]

hound · 2018-12-06T16:54:40Z

spec/scenic/statements_spec.rb

@@ -43,9 +43,9 @@ module Scenic
          with(:views, definition_stub.to_sql)
      end

-      it "raises an error if both version and sql_defintion are provided" do
+      it "raises an error if both version and sql_definition are provided" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:54:40Z

spec/scenic/statements_spec.rb


        connection.create_view(:views, sql_definition: sql_definition)

        expect(Scenic.database).to have_received(:create_view)
          .with(:views, sql_definition)
      end

-      it "creates version 1 of the view if neither version nor sql_defintion are provided" do
+      it "creates version 1 of the view if neither version nor sql_definition are provided" do


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Metrics/LineLength: Line is too long. [94/80]

hound · 2018-12-06T16:54:40Z

spec/scenic/statements_spec.rb

@@ -22,15 +22,15 @@ module Scenic
      end

      it "creates a view from a text definition" do
-        sql_definition = "a defintion"
+        sql_definition = "a definition"


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2018-12-06T16:54:40Z

lib/scenic/definition.rb

        if content.empty?
          raise "Define view query in #{path} before migrating."
        end
      end
    end

+    def definition_path
+      resolve_definition || raise("Unable to locate #{filename} in #{views_paths}")


Metrics/LineLength: Line is too long. [83/80]

vladimirtemnikov · 2019-04-02T21:17:16Z

Hi, really waiting for this 👍

tomasc · 2019-07-18T09:29:25Z

Hi, would be great to have this merged! Just ran into this when trying to use scenic from engines.

derekprior · 2019-07-18T11:37:04Z

Rails 6 adds native support for multiple databases. At this point, I’m inclined to see how we would be able to work with that.

tomasc · 2019-07-18T11:43:02Z

@derekprior an alternative approach would be to make the full_path used in definition.rb configurable – by way of being able to pass root_path in the configuration:

Scenic.configure do |config|
  config.root_path = MyEngine.root
end

In any case, the current setup prevents the gem from being used from within Rails engines as it looks up the db/views under the engine's test app. This is not related to multi-database support in Rails 6 I believe.

Would you be interested in a PR implementing the above?

huarmenta · 2019-07-22T17:35:27Z

I agree with @tomasc , it would be nice to have a configuration called root_path like the one that @tomasc propose or views_path to be more specific to set the views path, i thinks this is a better approach than the one on this PR because it is more flexible than overriding a definition.

@derekprior Any provisional solution on how to use scenic inside an engine without copying files?

derekprior · 2021-12-15T23:17:53Z

Long time no review on this PR. We've had a number of PRs for this feature (see view-paths ) so I think it merits further consideration. I think this is the closest to something we could accept as it supports multiple search paths. I hope to look at this closer in the next couple of weeks.

smdern added 3 commits February 13, 2018 20:55

Improve find_definition spec

7bd3077

Show expanded paths in views_paths

97f40b2

houndci-bot reviewed Feb 14, 2018

View reviewed changes

spec/scenic/definition_spec.rb Outdated Show resolved Hide resolved

spec/scenic/definition_spec.rb Outdated Show resolved Hide resolved

spec/scenic/definition_spec.rb Outdated Show resolved Hide resolved

Fix lint errors

b0c56b4

smdern changed the title ~~Support multiple views paths~~ Support multiple db/views paths Feb 14, 2018

derekprior reviewed Jun 1, 2018

View reviewed changes

lib/scenic/definition.rb Outdated Show resolved Hide resolved

lib/scenic/definition.rb Outdated Show resolved Hide resolved

spec/scenic/definition_spec.rb Outdated Show resolved Hide resolved

calebhearth requested changes Dec 5, 2018

View reviewed changes

nathanpalmer mentioned this pull request Dec 6, 2018

Use scenic within a rails engine / component #261

Open

Change find_defintion to defintion_path

4bfafd8

hound bot reviewed Dec 6, 2018

View reviewed changes

Merge branch 'master' into support-multiple-views-paths

168769f

smdern added 2 commits December 6, 2018 16:53

Fix all defintion -> definition typos

ff63ad0

Extract resolve_definition into a private method

ff5382a

hound bot reviewed Dec 6, 2018

View reviewed changes

smdern force-pushed the support-multiple-views-paths branch from a7817a3 to ff5382a Compare December 6, 2018 17:07

derekprior changed the base branch from master to main June 18, 2020 15:03

tomasc mentioned this pull request Sep 16, 2020

Configurable root path #306

Closed

derekprior added the view-paths label Dec 15, 2021

derekprior mentioned this pull request Dec 15, 2021

allow to configure definition path #321

Closed

2 tasks

derekprior mentioned this pull request Dec 15, 2021

Implement custom views path #298

Closed

calebhearth added this to the 2.0 milestone Jan 20, 2024

Support multiple db/views paths #237

Are you sure you want to change the base?

Support multiple db/views paths #237

Conversation

smdern commented Feb 14, 2018

coding-chimp commented Apr 25, 2018 • edited Loading

MarcReniu commented Jun 1, 2018

derekprior left a comment

Choose a reason for hiding this comment

calebhearth Dec 5, 2018

Choose a reason for hiding this comment

smdern Dec 6, 2018

Choose a reason for hiding this comment

calebhearth Dec 5, 2018

Choose a reason for hiding this comment

calebhearth Dec 5, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

smdern commented Dec 6, 2018

smdern commented Dec 6, 2018

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

hound bot Dec 6, 2018

Choose a reason for hiding this comment

vladimirtemnikov commented Apr 2, 2019

tomasc commented Jul 18, 2019

derekprior commented Jul 18, 2019

tomasc commented Jul 18, 2019

huarmenta commented Jul 22, 2019

derekprior commented Dec 15, 2021

coding-chimp commented Apr 25, 2018 •

edited

Loading