alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
197 stars 77 forks source link

Add code to require all Ruby files in and under lib/dnsruby. #77

Closed keithrbennett closed 9 years ago

keithrbennett commented 9 years ago

We have some explicit requires (requires of specific files) in lib/dnsruby.rb.

While there may be some recursive requiring going on, not all the files are required as a result. For example:

2.1.4 :001 > require 'dnsruby'
 => true
2.1.4 :002 > Dnsruby::Resolv
NameError: uninitialized constant Dnsruby::Resolv

This code modification requires all files in lib/dnsruby and below. This is the simplest way to guarantee that all files are included. The expansion of the wild card will also include the files already explicitly required, but require will notice this and not try to require the file again, so this is not a problem.

alexdalitz commented 9 years ago

Is this not a bit crude?

Should we not just ensure that we include the classes that are part of the interface?

Thanks,

Alex.

keithrbennett commented 9 years ago

Regarding Resolv specifically, I was going to say that Resolv would be loaded by some file at run time anyway, but when I examined the code, it looks like it's only used by the test code. That being the case, can we move Resolv to the test directory tree, or do we still need it in lib, and, if so, why?

Regarding the general practice of brute force requiring all .rb files in a gem, I think it depends.

Which files do we want to exclude? If those files will be loaded by other files that rely on them anyway, what value is there in excluding it from the top level require?

I agree that it's kind of a hack. Ideally, we would have each file require every file that it needs, minus some that are dependencies that would be loaded by the required files already -- but we don't have that anyway. We have instead a (probably) partial require of the library hardcoded into dnsruby.rb. And IIRC we've run into omissions of requires when loading some files individually as opposed to via the gem, as in unit tests.

So one argument would be that, in the absence of the will and the time to specify all requires correctly, a kludge that would be effective in all files being required would be a helpful thing.

I have had situations where I wanted to exclude certain files from being required, when some files were MRI Ruby only, and others were JRuby only. In those cases, I made it explicit in the code that I was excluding them, and why. Another good reason to exclude files is if they require a substantial amount of memory and are not always necessary. But excluding a file from being required to indicate that it's private...I don't know if that makes sense. It needs to be required somewhere for the VM to execute it. But I may be missing something.

I don't know. I've been on both sides of this fence.

On Mon, Mar 9, 2015 at 5:34 PM, Alex Dalitz notifications@github.com wrote:

Is this not a bit crude?

Should we not just ensure that we include the classes that are part of the interface?

Thanks,

Alex.

— Reply to this email directly or view it on GitHub https://github.com/alexdalitz/dnsruby/pull/77#issuecomment-77948016.

alexdalitz commented 9 years ago

Resolv is the top-level interface to our code.

It is used by client code, and test code - which is why it does not feature much in the source tree.

It is important that it is included - but, this will normally be by the client themselves.

I’m happy to add it as a specific include in the dnsruuby code - but this patch seems a heavyweight way to do that!

Thanks!

Alex.

On 9 Mar 2015, at 22:21, Keith Bennett <notifications@github.com mailto:notifications@github.com> wrote:

Regarding Resolv specifically, I was going to say that Resolv would be loaded by some file at run time anyway, but when I examined the code, it looks like it's only used by the test code. That being the case, can we move Resolv to the test directory tree, or do we still need it in lib, and, if so, why?

Regarding the general practice of brute force requiring all .rb files in a gem, I think it depends.

Which files do we want to exclude? If those files will be loaded by other files that rely on them anyway, what value is there in excluding it from the top level require?

I agree that it's kind of a hack. Ideally, we would have each file require every file that it needs, minus some that are dependencies that would be loaded by the required files already -- but we don't have that anyway. We have instead a (probably) partial require of the library hardcoded into dnsruby.rb. And IIRC we've run into omissions of requires when loading some files individually as opposed to via the gem, as in unit tests.

So one argument would be that, in the absence of the will and the time to specify all requires correctly, a kludge that would be effective in all files being required would be a helpful thing.

I have had situations where I wanted to exclude certain files from being required, when some files were MRI Ruby only, and others were JRuby only. In those cases, I made it explicit in the code that I was excluding them, and why. Another good reason to exclude files is if they require a substantial amount of memory and are not always necessary. But excluding a file from being required to indicate that it's private...I don't know if that makes sense. It needs to be required somewhere for the VM to execute it. But I may be missing something.

I don't know. I've been on both sides of this fence.

  • Keith

On Mon, Mar 9, 2015 at 5:34 PM, Alex Dalitz <notifications@github.com mailto:notifications@github.com> wrote:

Is this not a bit crude?

Should we not just ensure that we include the classes that are part of the interface?

Thanks,

Alex.

— Reply to this email directly or view it on GitHub <https://github.com/alexdalitz/dnsruby/pull/77#issuecomment-77948016 https://github.com/alexdalitz/dnsruby/pull/77#issuecomment-77948016>.

— Reply to this email directly or view it on GitHub https://github.com/alexdalitz/dnsruby/pull/77#issuecomment-77956996.

Alex Dalitz alex@caerkettontech.com mailto:alex@caerkettontech.com

keithrbennett commented 9 years ago

Ok, I will relent. ;) I've deleted the recursive require and have added instead an explicit:

require 'dnsruby/resolv'
coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.04%) to 77.87% when pulling e0a86f3fd9fa002deaaea59939667aa932d21dca on keithrbennett:require_all_files into 894a64184fe02075cbb1f9f6afec13ac793aef7d on alexdalitz:master.