davetron5000 / gli

Make awesome command-line applications the easy way
http://davetron5000.github.io/gli
Apache License 2.0
1.26k stars 102 forks source link

lib/gli/version.rb VERSION context #258

Closed micah closed 3 years ago

micah commented 7 years ago

Hi,

Any code that loads your gemspec already has a VERSION in its context because in that context const_defined? :VERSION will return true if either GLI::VERSION is already defined, or if there is already a VERSION constant defined at the top level.

For example:

$ ruby -e 'VERSION = 1; Gem::Specification.load("gli.gemspec")'
Invalid gemspec in [gli.gemspec]: uninitialized constant GLI::VERSION
Did you mean?  VERSION

Since the code in lib/gli/version.rb is always loaded via require, there is no need to test for an existing constant, since require guarantess that no file is loaded twice (e.g. if you do require "foo" twice, the second one is a no-op).

It would be better if lib/gli/version.rb was changed to the following:

--- a/lib/gli/version.rb
+++ b/lib/gli/version.rb
@@ -1,5 +1,3 @@
 module GLI
-  unless const_defined? :VERSION
-    VERSION = '2.14.0'
-  end
+  VERSION = '2.14.0'
 end
davetron5000 commented 7 years ago

According to 2ce9097a8242d80faf27c6ae7e0a3a6f26569317 that needs to be there for tests, which use a different path to the file given to require. require hashes on the argument not the full path (or it did as of 2011 :).

If removing that doesn't introduce warnings on supported rubies, it's fine with me, but it was needed at some point.

terceiro commented 7 years ago

well that is not the case for a while ...

$ irb -Ilib
irb(main):001:0> RUBY_VERSION
=> "2.1.5"
irb(main):002:0> require 'gli/version'
=> true
irb(main):003:0> require 'gli/version.rb'
=> false
irb(main):004:0> require './lib/gli/version.rb'
=> false
irb(main):005:0> require Dir.pwd + '/lib/gli/version.rb'
=> false

(require returns false when the code you are trying to load is already loaded)

terceiro commented 7 years ago

also, requiring code by its explicit path in tests is an anti-pattern that causes other issues, as when you are trying to run the tests against the library that is installed rather than the one in the source directory.

davetron5000 commented 3 years ago

Closing as old and I'm not 100% sure I understand the issue any longer. I don’t see warnings in the test output