Fix circular require #183
Fix circular require #183
Conversation
davebenvenuti
requested review from
rwstauner,
nirvdrum and
tenderworks
and removed request for
rwstauner
|
|
||
| private | ||
|
|
||
| def assert_rb_runs_successfully(ruby, msg = "Ruby returned non-zero exit status") |
davebenvenuti
added
the
#gsd:43107
Co-authored-by: Randy Stauner <[email protected]>
I know we talked about this offline a bit, but I'm not sure what the convention is here. One option for autoloading looks like this: diff --git a/lib/protoboeuf.rb b/lib/protoboeuf.rb
index bd3a0c6..b3c2dcd 100644
--- a/lib/protoboeuf.rb
+++ b/lib/protoboeuf.rb
@@ -3,4 +3,6 @@
module ProtoBoeuf
autoload :CodeGen, "protoboeuf/codegen"
autoload :Google, "protoboeuf/google"
+
+ autoload :TypeHelper, "protoboeuf/type_helper"
end
diff --git a/lib/protoboeuf/codegen.rb b/lib/protoboeuf/codegen.rb
index b28ebcc..18f6f9b 100644
--- a/lib/protoboeuf/codegen.rb
+++ b/lib/protoboeuf/codegen.rb
@@ -9,7 +9,7 @@ module ProtoBoeuf
class EnumCompiler
attr_reader :generate_types
- include TypeHelper
+ include ProtoBoeuf::TypeHelper
class << self
def result(enum, generate_types:, options: {})
@@ -79,7 +79,7 @@ module ProtoBoeuf
class MessageCompiler
attr_reader :generate_types, :requires
- include TypeHelper
+ include ProtoBoeuf::TypeHelper
class << self
def result(message, toplevel_enums, generate_types:, requires:, syntax:, options: {})The reason for fully qualifying the module name in the On the one hand, autoloading the module makes things a little more consistent. On the other, the purpose of autoload is to avoid requiring dependencies that we don't end up using if certain code paths aren't executed. In the case of Another option is to move the diff --git a/lib/protoboeuf/codegen.rb b/lib/protoboeuf/codegen.rb
index b28ebcc..43188c3 100644
--- a/lib/protoboeuf/codegen.rb
+++ b/lib/protoboeuf/codegen.rb
@@ -2,9 +2,10 @@
require "erb"
require "syntax_tree"
-require_relative "type_helper"
module ProtoBoeuf
+ autoload :TypeHelper, "protoboeuf/type_helper"
+
class CodeGen
class EnumCompiler
attr_reader :generate_typesThis is a little better, because now the dependency of I'm leaning towards keeping is as a |
test/gem_test.rb
Outdated
|
|
||
| ::ProtoBoeuf | ||
|
|
||
| # The following should autoloaded |
There was a problem hiding this comment.
Should we assert these autoload using Module#autoload??
There was a problem hiding this comment.
This change isn't technically related to this PR. The only reason it shows up in this diff is because I moved the Tempfile stuff we were doing here to the private assert_rb_runs_successfully helper method. But yeah that's probably worth verifying.
|
|
||
| refute_match(/warning: loading in progress, circular require considered harmful/, stderr) |
There was a problem hiding this comment.
I like being specific, but I'm a bit worried that any change in the warning message could cause this test to pass unexpectedly. These messages aren't really stable across Ruby versions. I'm not sure if JRuby or TruffleRuby even emit them identically.
There was a problem hiding this comment.
Would you suggest refuting a match on a subset of the message, like /circular require/? I suppose that's more likely to be consistent across platforms and versions. Our generated code currently produces other warnings so simply asserting that we don't have any warnings won't work I'm afraid.
|
I think we can just leave it as require_relative since this module is going to use it and there probably isn't anything else that will request it. |
Fixes: #173
There's no need for
TypeHelperto be namespaced under theCodeGenclass if its intent is to be a reusable module. This PR renames the file to match the module name and puts it directly under theProtoBoeufmodule.