New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix reloader issue with some gems that redefine Class#name (like pry) #807

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@dcu
Contributor

dcu commented Mar 16, 2012

padrino start doesn't work when using 'pry'

fix reloader issue with some gems that redefine Class#name (like pry)
Signed-off-by: David A. Cuadrado <krawek@gmail.com>
@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Mar 18, 2012

Yep, we reverted back to name because some libs redefine also to_s ... so suggestions?

DAddYE commented on 9e7b99b Mar 18, 2012

Yep, we reverted back to name because some libs redefine also to_s ... so suggestions?

DAddYE added a commit that referenced this pull request Mar 18, 2012

make sure we can always use the original class name
Signed-off-by: David A. Cuadrado <krawek@gmail.com>
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 4, 2012

Member

@DAddYE Should we still merge this or is the commit you put in fix it as well?

Member

nesquena commented Apr 4, 2012

@DAddYE Should we still merge this or is the commit you put in fix it as well?

@dcu

This comment has been minimized.

Show comment
Hide comment
@dcu

dcu Apr 4, 2012

Contributor

this issue is critic... you should release a version to fix this as soon as possible.

Contributor

dcu commented Apr 4, 2012

this issue is critic... you should release a version to fix this as soon as possible.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 4, 2012

Member

@dcu Does our version solve it 5c2623e or do we still need this?

Member

nesquena commented Apr 4, 2012

@dcu Does our version solve it 5c2623e or do we still need this?

@dcu

This comment has been minimized.

Show comment
Hide comment
@dcu

dcu Apr 4, 2012

Contributor

according to @DAddYE it was .to_s before changing it to .name. his change only reverts back to the original(to_s) behaviour which, I understand, doesn't work because some libs override Class|Module#to_s
I think I explain well in the commit comments why that change won't work

my commit guarantees that it will always work in all cases

Contributor

dcu commented Apr 4, 2012

according to @DAddYE it was .to_s before changing it to .name. his change only reverts back to the original(to_s) behaviour which, I understand, doesn't work because some libs override Class|Module#to_s
I think I explain well in the commit comments why that change won't work

my commit guarantees that it will always work in all cases

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 4, 2012

Member

OK, let's just apply your change. Does that work @DAddYE

Member

nesquena commented Apr 4, 2012

OK, let's just apply your change. Does that work @DAddYE

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 5, 2012

Member

OK, well I applied your version here: 865d67c. Does that look OK @dcu? Thanks for your help in all this. I plan to release a new version soon.

Member

nesquena commented Apr 5, 2012

OK, well I applied your version here: 865d67c. Does that look OK @dcu? Thanks for your help in all this. I plan to release a new version soon.

@nesquena nesquena closed this Apr 5, 2012

@dcu

This comment has been minimized.

Show comment
Hide comment
@dcu

dcu Jun 8, 2012

Contributor

you should start doing faster release cycles. if you don't want to do a 0.10.7 why dont you release 0.10.6.1 ?

Contributor

dcu commented Jun 8, 2012

you should start doing faster release cycles. if you don't want to do a 0.10.7 why dont you release 0.10.6.1 ?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 11, 2012

Member

@DAddYE What do you think? Perhaps we should release either what we have as 0.10.7 or do as dcu suggests and just push a 0.10.6.1 at least?

Member

nesquena commented Jun 11, 2012

@DAddYE What do you think? Perhaps we should release either what we have as 0.10.7 or do as dcu suggests and just push a 0.10.6.1 at least?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jun 11, 2012

Member

I agree we can release 0.10.7

Member

DAddYE commented Jun 11, 2012

I agree we can release 0.10.7

@exolab

This comment has been minimized.

Show comment
Hide comment
@exolab

exolab Jun 15, 2012

When is the release due?

exolab commented Jun 15, 2012

When is the release due?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jun 15, 2012

Member

We are waiting @nesquena and @achiu confirmations

Member

DAddYE commented Jun 15, 2012

We are waiting @nesquena and @achiu confirmations

@argent-smith

This comment has been minimized.

Show comment
Hide comment
@argent-smith

argent-smith Jun 16, 2012

C'mon guys! You can!

argent-smith commented Jun 16, 2012

C'mon guys! You can!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 20, 2012

Member

@argent-smith Yay we can and we have, pushed 0.10.7 finally. Sorry everyone for the delays!

Member

nesquena commented Jun 20, 2012

@argent-smith Yay we can and we have, pushed 0.10.7 finally. Sorry everyone for the delays!

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