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

Use Lotus::String#deconstantize to detect application_module #67

Closed
wants to merge 3 commits into from
Closed

Use Lotus::String#deconstantize to detect application_module #67

wants to merge 3 commits into from

Conversation

rail44
Copy link

@rail44 rail44 commented Sep 2, 2014

Hi.

On defining following Application.

module Hoge
  module V1
    class Application < Lotus::Application
      ...
    end
  end
end

Lotus::Loader will load framework to toplevel module(Hoge) by this line
https://github.com/lotus/lotus/blob/41d1fe8624b5f26c111cc019283602fedc8fd237/lib/lotus/loader.rb#L98

This PR make the Loader will load framework into application's own parent module.

It depends on hanami/utils#28

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cac6fcc on rail44:modify-loader-application-module into * on lotus:master*.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ce49ecf on rail44:modify-loader-application-module into 38d9818 on lotus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling bddd591 on rail44:modify-loader-application-module into 38d9818 on lotus:master.

@rail44
Copy link
Author

rail44 commented Sep 6, 2014

Is test enough?

And I've found that this PR make some of components have to be fixed.
e.g.
https://github.com/lotus/controller/blob/master/lib/lotus/controller/configuration.rb#L91
this line should use deconstantize.

@jodosha
Copy link
Member

jodosha commented Sep 19, 2014

@rail44 Thanks for this PR, I think that this is the way to go.

As a developer who wants to use Lotus, I expect that my controllers are nested under Hoge::V1 instead of Hoge.

However, I don't like the implementation. Utils::String#namespace was created to support Lotus applications namespaces. Until now, it supports only one level of depth. In my opinion, we should add more levels, but enhancing that method instead of introducing a new one (#deconstantize).

The proof of what I'm saying is this code:

def application_module
  if @application_module
    @application_module
  else
    deconstantize = Utils::String.new(application.name).deconstantize
    if deconstantize.empty?
      @application_module = application.class
    else
      @application_module = Utils::Class.load!(deconstantize)
    end
end

In the code above, we are moving the decision of which Ruby namespace to pick outside of two methods that supposed to take that decision. Other parts of the framework need to use this new logic, if we don't move that logic into Utils::String#namespace we are forced to repeat this if condition everywhere.

What to do from now:

  1. Change Add Lotus::String#deconstantize utils#28 to replicate the same behavior but in #namespace.
  2. Change this PR by eliminating that if and using #namespace.

In this way, the other frameworks can stay untouched, because they will keep to use the same method (eg see Lotus::Controller::Configuration.for).

@jodosha jodosha self-assigned this Sep 19, 2014
@rail44
Copy link
Author

rail44 commented Oct 5, 2014

@jodosha
Probably, we cannot keep Configuration.for untouched if we impl this feature.
Because, this method tries to find configuration with believing Controller or View is lies on top level, and it sometimes called with Action.

I comes up with following way for this issue,

# Utils::String
def parents
  parts = split(NAMESPACE_SEPARATOR)[0...-1]
  parts.length.downto(1).map do |i|
    self.class.new parts.slice(0, i).join(NAMESPACE_SEPARATOR)
  end
end

# Configuration
def self.for(base)
  modules = Utils::String.new(base).parents << 'Lotus'
  framework = Utils::Class.load("(#{modules.join('|')})::Controller")
  framework.configuration.duplicate
end

How do you think about it?
I am getting realized that it is not bad to give up supporting nested module.

@rail44 rail44 closed this Oct 5, 2014
@rail44 rail44 reopened this Oct 5, 2014
@jodosha
Copy link
Member

jodosha commented Oct 6, 2014

@rail44 I looked at the parts involved with this feature: it requires Model, View & Controller's Configuration.for to change. I made it working for Controller, but the implementation is hacky, and has other kind of runtime restrictions: create a registry for all the configurations, forbid to modify a configuration after it's being registered etc..

Because this is a nice to have feature for now, and not a priority, the effort of changing all the frameworks it's not worth for now.

Let's stick with:

Bookshelf::Application # OK
Boohshelf::Backend # OK
Boohshelf::Frontend # OK
Boohshelf::Web # OK
Boohshelf::Whatever # OK

Boohshelf::Backend::Application # NOT OK

Of course, if you find a clean solution, I'll be happy to merge it. Thank you!

@jodosha jodosha closed this Oct 6, 2014
@rail44
Copy link
Author

rail44 commented Oct 7, 2014

ty!

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.

None yet

3 participants