Only remove a constant if it exists. #354

Merged
merged 3 commits into from Mar 7, 2012

Conversation

Projects
None yet
4 participants
@benhamill
Contributor

benhamill commented Mar 4, 2012

This is a modification of an earlier pull request #247. I'm sort of surprised that so many people typed the exact code I did here into comments on that thread, rather than forking and issuing a pull request themselves. So, uh... here's this. I think it fixes some problem people using 1.9.1 were having, but honestly I didn't test it; I just implemented what people said would fix their problem in comments. Would love some feedback if this should be done differently.

@lgierth

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Mar 4, 2012

Contributor

Am I missing something or doesn't the line right above make the conditional obsolete as WKFV_ will definitely not be defined?

Contributor

lgierth commented Mar 4, 2012

Am I missing something or doesn't the line right above make the conditional obsolete as WKFV_ will definitely not be defined?

@benhamill

This comment has been minimized.

Show comment Hide comment
@benhamill

benhamill Mar 4, 2012

Contributor

hahaha. Damnit. Yes. Thanks! My conditional should be on THAT line. 😣

Contributor

benhamill commented Mar 4, 2012

hahaha. Damnit. Yes. Thanks! My conditional should be on THAT line. 😣

@lgierth

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Mar 4, 2012

Contributor

Ha, makes sense :D

Contributor

lgierth commented Mar 4, 2012

Ha, makes sense :D

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 4, 2012

Member

Thanks for looking into this. I think to make it perfect, you should use const_defined? there.

Member

rkh commented Mar 4, 2012

Thanks for looking into this. I think to make it perfect, you should use const_defined? there.

@benhamill

This comment has been minimized.

Show comment Hide comment
@benhamill

benhamill Mar 4, 2012

Contributor

I don't have the docs in front of me, but I thought const_defined? was private on Object, so unless the constant is defined within the module you're calling it, you have to use send, which seemed sloppy. I could be wrong.

On Mar 4, 2012, at 5:02 AM, Konstantin Haasereply@reply.github.com wrote:

Thanks for looking into this. I think to make it perfect, you should use const_defined? there.


Reply to this email directly or view it on GitHub:
#354 (comment)

Contributor

benhamill commented Mar 4, 2012

I don't have the docs in front of me, but I thought const_defined? was private on Object, so unless the constant is defined within the module you're calling it, you have to use send, which seemed sloppy. I could be wrong.

On Mar 4, 2012, at 5:02 AM, Konstantin Haasereply@reply.github.com wrote:

Thanks for looking into this. I think to make it perfect, you should use const_defined? there.


Reply to this email directly or view it on GitHub:
#354 (comment)

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 4, 2012

Member

const_defined? is public. Interestingly, remove_const is private.

Member

rkh commented Mar 4, 2012

const_defined? is public. Interestingly, remove_const is private.

@judofyr

This comment has been minimized.

Show comment Hide comment
@judofyr

judofyr Mar 6, 2012

Contributor

What's the problem with defined??

Contributor

judofyr commented Mar 6, 2012

What's the problem with defined??

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 6, 2012

Member

It also checks for constants in the lexical scope (as opposed to just inherited constants), methods, etc., so I like to avoid it for simple constant checks if possible.

Member

rkh commented Mar 6, 2012

It also checks for constants in the lexical scope (as opposed to just inherited constants), methods, etc., so I like to avoid it for simple constant checks if possible.

@benhamill

This comment has been minimized.

Show comment Hide comment
@benhamill

benhamill Mar 6, 2012

Contributor

I get that it's checking more than just constants. For the purpose of expanding my understanding, can you clarify the distinction you're drawing between "constants in the lexical scope" and "inherited constants"? It may just be a jargon thing, or I may have a flawed model of scope or... something.

Contributor

benhamill commented Mar 6, 2012

I get that it's checking more than just constants. For the purpose of expanding my understanding, can you clarify the distinction you're drawing between "constants in the lexical scope" and "inherited constants"? It may just be a jargon thing, or I may have a flawed model of scope or... something.

@rkh

This comment has been minimized.

Show comment Hide comment
@rkh

rkh Mar 6, 2012

Member
module Outer
  Foo = 10
  module Inner
    p defined? Foo
    p const_defined? :Foo
  end
end

I think this was a bit of yak shaving on my part, but I have this inner instinct to always prefer const_defined? over defined? if possible

Member

rkh commented Mar 6, 2012

module Outer
  Foo = 10
  module Inner
    p defined? Foo
    p const_defined? :Foo
  end
end

I think this was a bit of yak shaving on my part, but I have this inner instinct to always prefer const_defined? over defined? if possible

@benhamill

This comment has been minimized.

Show comment Hide comment
@benhamill

benhamill Mar 6, 2012

Contributor

Huh! I'd never realized that difference. For those too lazy to pop open irb (for which I don't blame you), in the example, defined? returns the "constant" and const_defined? returns false. It might be a bit of a yak shave, but in this case, I think we'd rather see exceptions if WFKV_ is defined, but not inside the URI namespace. I'll push the change tonight.

Contributor

benhamill commented Mar 6, 2012

Huh! I'd never realized that difference. For those too lazy to pop open irb (for which I don't blame you), in the example, defined? returns the "constant" and const_defined? returns false. It might be a bit of a yak shave, but in this case, I think we'd rather see exceptions if WFKV_ is defined, but not inside the URI namespace. I'll push the change tonight.

@benhamill

This comment has been minimized.

Show comment Hide comment
@benhamill

benhamill Mar 7, 2012

Contributor

There. That's better.

Contributor

benhamill commented Mar 7, 2012

There. That's better.

rkh added a commit that referenced this pull request Mar 7, 2012

@rkh rkh merged commit 8d2cb75 into rack:master Mar 7, 2012

@judofyr

This comment has been minimized.

Show comment Hide comment
@judofyr

judofyr Mar 7, 2012

Contributor

@rkh: Of course. I first thought it had something to do with "defined? is super magic syntax", but that makes sense.

Contributor

judofyr commented Mar 7, 2012

@rkh: Of course. I first thought it had something to do with "defined? is super magic syntax", but that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment