-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable extension developers to customize the namespace #3161
Enable extension developers to customize the namespace #3161
Conversation
9cb6bc7
to
f3e564a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specs are failing but that might just need a rebase from master
@@ -24,7 +24,7 @@ | |||
begin | |||
require "generators/#{ENV['LIB_NAME']}/install/install_generator" | |||
puts 'Running extension installation generator...' | |||
"#{ENV['LIB_NAME'].camelize}::Generators::InstallGenerator".constantize.start(["--auto-run-migrations"]) | |||
"#{ENV['LIB_NAMESPACE'] || ENV['LIB_NAME'].camelize}::Generators::InstallGenerator".constantize.start(["--auto-run-migrations"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does LIB_NAMESPACE
need to be camelize
as well? Should this be #{(ENV['LIB_NAMESPACE'] || ENV['LIB_NAME']).camelize}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericsaupe I don't think so, because LIB_NAMESPACE
is meant to be the Solidus extension gem namespace, it isn't used as a file path, so it doesn't make sense to pass it in a file path form; I meant to use LIB_NAMESPACE
in order to skip transformations, so f.e. in this way:
LIB_NAME=solidus_graphql_api LIB_NAMESPACE=SolidusGraphQLAPI bundle exec rake test_app
Do you think we could refactor in a better way the flow? I was thinking about that but unfortunately it didn't come me to mind anything better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! Thanks for the explanation.
f3e564a
to
0f73e06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks, @mdesantis!
@mdesantis can you please rebase against master now? |
@kennyadsl FYI I rebased with master a couple of hours ago and surprisingly tests are still failing; I'm going to try again |
If you are developing an extension and you want to use `Rake::Task['extension:test_app']` you are forced to have a file named exactly as the capitalized version of the extension namespace name, so if you name your namespace extension e.g. SolidusGraphQL you are forced to create a file named `solidus_graph_q_l`, otherwise the following error is raised: ``` > bundle exec rake test_app --trace ** Invoke default (first_time) ** Invoke first_run (first_time) ** Execute first_run ** Invoke test_app (first_time) ** Execute test_app ** Invoke extension:test_app (first_time) ** Execute extension:test_app ** Invoke common:test_app (first_time) ** Execute common:test_app rake aborted! LoadError: cannot load such file -- solidus_graph_q_l ``` This change allows the developer to customize the extension namespace setting a `LIB_NAMESPACE` environment variable.
0f73e06
to
8241630
Compare
I've just merged #3202 that should fix the build |
Description
If you are developing an extension and you want to use
Rake::Task['extension:test_app']
you are forced to have a file named exactly as the capitalized version of the extension namespace name, so if you name your namespace extension e.g. SolidusGraphQL you are forced to create a file namedsolidus_graph_q_l
, otherwise the following error is raised:This change allows the developer to customize the extension namespace setting a
LIB_NAMESPACE
environment variable.Checklist: