change to constantize, part 2 #175

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
@davebenvenuti

sorry, still haven't completely gotten the hang of pull requests yet. it appears i also have nirvdrum's changes in there too; you can ignore commit 1fc8a46. to reiterate:

I ran into an issue when trying to enqueue classes which were within modules that also appeared within a submodule. that's a mouthful; here's an example. my runtime defines the following:

module X::Y

class Z
end

module Y
end

when trying to enqueue a job with class X::Y::Z, i would get the error that it cannot constantize Y::Z. this is because in the implementation of constantize in helper.rb, when const_get was called on module X, it would return the higher level module Y, not X::Y. to fix this, i pulled in a newer implementation of constantize from activesupport that is more ruby 1.9-friendly. i also modified job so that it'll use the native implementation, if it exists (i happen to be loading the rails environment in my resque jobs. not sure how common this is, but i figured this would provide multiple safeguards against the problem)

I've tweaked the constantize method a bit since the last pull request, plus added a test for this case. I also updated the tests, which weren't running in Ruby 3 since the current directory was removed from the load path

@hobodave

This comment has been minimized.

Show comment
Hide comment
@hobodave

hobodave Dec 17, 2010

sorry, still haven't completely gotten the hang of pull requests yet. it appears i also have nirvdrum's changes in there too; you can ignore commit 1fc8a46

You should get the hang of branching actually. It's bad form to ask project maintainers to pull from your master branch, particularly when you've merged who knows how many other branches into it.

If you always do your work in a branch based off the upstream master, you can submit the pull request from that branch. This allows you to mutilate your master as you see fit, and makes it gobs easier for a maintainer to review, and subsequently incorporate via a simple merge. Otherwise it's a bit of a PITA to dig through the commits in your master branch and determine which need cherry-picking.

sorry, still haven't completely gotten the hang of pull requests yet. it appears i also have nirvdrum's changes in there too; you can ignore commit 1fc8a46

You should get the hang of branching actually. It's bad form to ask project maintainers to pull from your master branch, particularly when you've merged who knows how many other branches into it.

If you always do your work in a branch based off the upstream master, you can submit the pull request from that branch. This allows you to mutilate your master as you see fit, and makes it gobs easier for a maintainer to review, and subsequently incorporate via a simple merge. Otherwise it's a bit of a PITA to dig through the commits in your master branch and determine which need cherry-picking.

@nirvdrum

This comment has been minimized.

Show comment
Hide comment
@nirvdrum

nirvdrum Dec 17, 2010

Contributor

I understand this comment was well-intentioned, but the tone was fairly unhelpful. I know how to branch just fine and still get confused by pull requests, which as far as I can tell, are a GitHub invention. My guess is davebenvenuti ended up in a similar boat. By all accounts, he's gone well out of his way to try make this project better. I'd give him a pass on having to skip over a commit that ended up on defunkt's master anyway.

Contributor

nirvdrum commented Dec 17, 2010

I understand this comment was well-intentioned, but the tone was fairly unhelpful. I know how to branch just fine and still get confused by pull requests, which as far as I can tell, are a GitHub invention. My guess is davebenvenuti ended up in a similar boat. By all accounts, he's gone well out of his way to try make this project better. I'd give him a pass on having to skip over a commit that ended up on defunkt's master anyway.

@hobodave

This comment has been minimized.

Show comment
Hide comment
@hobodave

hobodave Dec 18, 2010

You're right nirvdrum; I was rather snarky in my comment. My apologies to both you and davebenvenuti.

You're right nirvdrum; I was rather snarky in my comment. My apologies to both you and davebenvenuti.

@defunkt

This comment has been minimized.

Show comment
Hide comment
@defunkt

defunkt Mar 17, 2011

Contributor

Sorry for the delay.

Are these the main changes here:

  1. constant.const_defined?(name, false) in 1.9, constant.const_defined?(name) in 1.8
  2. object.constantize if the object has the method
Contributor

defunkt commented Mar 17, 2011

Sorry for the delay.

Are these the main changes here:

  1. constant.const_defined?(name, false) in 1.9, constant.const_defined?(name) in 1.8
  2. object.constantize if the object has the method

@defunkt defunkt closed this Mar 18, 2011

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