QOL make our gem requireable and autoload/eager load some constants #171
Conversation
davebenvenuti
force-pushed
the
davebenvenuti/make-require-protoboeuf-work
branch
from
cafa5bd to
ecdc7b5
Compare
lib/protoboeuf/google.rb
Outdated
|
|
||
| # There isn't a clean 1:1 mapping between constants and *.rb files, so eager load instead of autoload. | ||
|
|
||
| Dir[File.expand_path("google/**/*.rb", __dir__)].each { |file| require file } |
There was a problem hiding this comment.
I'd prefer we don't do this pattern, and rather generate the requires as part of our .proto to .rb step.
2 reasons:
- It's important to know what we're requiring
- I'm not sure if directory listings are sorted (they weren't in the past) and it can cause platform dependent problems.
If generating a file with the requires is to onerous, I can understand and we can go this route, but I'd prefer if we didn't.
There was a problem hiding this comment.
I'll give that a whirl
There was a problem hiding this comment.
Added a new commit: 7dab057
We now generate modules in lib/protoboeuf/google/api.rb and lib/protoboeuf/google/protobuf.rb that autoload all of the constants defined in lib/protoboeuf/google/**/*.rb! Note that they're collapsed by default in the PR diff view because I marked them as autogenerated in .gitattributes. Big thanks to @rwstauner for helping me update the Rakefile in a way where I wouldn't embarrass myself.
davebenvenuti
force-pushed
the
davebenvenuti/make-require-protoboeuf-work
branch
8 times, most recently
from
5e8c636 to
a4ea5b8
Compare
…r generated constants
davebenvenuti
force-pushed
the
davebenvenuti/make-require-protoboeuf-work
branch
from
a4ea5b8 to
7dab057
Compare
| @parent_module_parts = parent_module.split("::") | ||
|
|
||
| # Given lib/protoboeuf/google.rb, glob lib/protoboeuf/google/**/*.rb | ||
| @child_ruby_filenames = Dir[module_filename.pathmap("%X/**/*.rb")].sort |
There was a problem hiding this comment.
This String#pathmap comes from rake.
Can we simplify that to get rid of that dependency?
Maybe just Dir["#{module_filename.delete_suffix(".rb")}/**/*.rb"] would be enough?
| child_constants = constants_for_child_ruby_filename(filename) | ||
| # For the autoloader_full_module_name we can just pick the first child constant we come across and take the | ||
| # first three parts. For example, ProtoBoeuf::Google::Api::FieldBehavior would be ProtoBoeuf::Google::Api. | ||
| if @autoloader_module_name.nil? |
| # For the autoloader_full_module_name we can just pick the first child constant we come across and take the | ||
| # first three parts. For example, ProtoBoeuf::Google::Api::FieldBehavior would be ProtoBoeuf::Google::Api. | ||
| if @autoloader_module_name.nil? | ||
| autoloader_full_module_name = child_constants.first.split("::")[0..2].join("::") |
There was a problem hiding this comment.
Can we generalize this 2 in case there's ever a different level of nesting?
What do we really want here, everything but the last one?
or one more than whatever is in parent_module?
| end | ||
| end | ||
|
|
||
| @generated_autoloader_module_parts = autoloader_full_module_name.split("::") |
There was a problem hiding this comment.
Do we use this value for something else, or are we joining it just to split it again?
Is there a better way to store this?
Pairing with @nirvdrum , we noticed
require "protoboeuf"didn't work despite having it in ourGemfilewhich led to some confusion (#170). This makesrequire "protoboeuf"work and auto- and eagerloads some constants for the user.ProtoBoeuf::CodeGenis nowautoloaded from the baselib/protoboeuf.rbProtoBoeuf::Googleis also nowautoloaded from the baselib/protoboeuf.rbEverything inlib/protoboeuf/google/**/*.rbis eager loaded whenProtoBoeuf::Googleloads. There isn't a clean 1:1 mapping between constants and*.rbfiles soautoloadwould be fairly tricky. Although this could be a controversial decision, I think this makes things easier to work with. But feedback is welcome.rake well_known_typesnow generates modules that autoload all child constants for well known types (eg:ProtoBoeuf::Google::Protobufwith the help ofProtoBoeuf::AutoloaderGen!