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

Please remove the ActiveSupport warning #1476

Open
DannyBen opened this Issue Sep 20, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@DannyBen

DannyBen commented Sep 20, 2018

if !$LOAD_PATH.grep(%r{gems/activesupport}).empty? && $LOADED_FEATURES.grep(%r{active_support/core_ext/hash}).empty?
puts <<-EOF
WARNING: If you plan to load any of ActiveSupport's core extensions to Hash, be
sure to do so *before* loading Sinatra::Application or Sinatra::Base. If not,
you may disregard this warning.
EOF
end

Sinatra is used a lot as an embedded server. Now some of the tools that I have developed display this warning in the end result (i.e. to the user, not the developer).

I am not even including ActiveSupport directly in my gem, it is included through sinatra-contrib....

This type of message is suitable for documentation, not for runtime in my opinion.

If you disagree, at least please give a developer a way to disable it, please...

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 20, 2018

Member

If you disagree, at least please give a developer a way to disable it, please...

Would an environment variable be an acceptable way of doing this?

Member

mwpastore commented Sep 20, 2018

If you disagree, at least please give a developer a way to disable it, please...

Would an environment variable be an acceptable way of doing this?

@DannyBen

This comment has been minimized.

Show comment
Hide comment
@DannyBen

DannyBen Sep 20, 2018

If you ask me, this "fix" creates more issues than it solves, and users should not be required to do anything to make it stop showing a warning, that they can do nothing about...

Don't get me wrong, I have mad respect for all the free time people put into open source, and Sinatra is one of my favorite Ruby projects, but - as an open source developer myself - I think we all need to remember that people are depending on what we do, and printing developer related warnings to stdout seems a little aggressive and out of place.

That said, if I failed to "pull you to my side of the table" - yes, an environment variable was what I had in mind, since it can be easily set up at the top level of anything that uses it, without touching any intermediate code layers.

DannyBen commented Sep 20, 2018

If you ask me, this "fix" creates more issues than it solves, and users should not be required to do anything to make it stop showing a warning, that they can do nothing about...

Don't get me wrong, I have mad respect for all the free time people put into open source, and Sinatra is one of my favorite Ruby projects, but - as an open source developer myself - I think we all need to remember that people are depending on what we do, and printing developer related warnings to stdout seems a little aggressive and out of place.

That said, if I failed to "pull you to my side of the table" - yes, an environment variable was what I had in mind, since it can be easily set up at the top level of anything that uses it, without touching any intermediate code layers.

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 20, 2018

Member

There are some horrifyingly subtle bugs that can happen here if you don't get the code ordering just so. It didn't make sense (to me) to bury the warning in a piece of documentation that no one will ever read. I agree that it is aggressive and ugly, but I disagree that it "creates more issues than it solves."

I'm definitely open to alternative solutions and will defer to the Sinatra team on this issue. An opt-out environment variable is probably the easiest and most portable "workaround," if a bit kludgy. We could only show the warning in dev and/or test. We could print to stderr instead of stdout. Or some combination of these.

A neat solution would be to defer the check until the app is completed booted and then compare the signatures of Hash and Sinatra::IndifferentHash to see if the former has been monkey-patched but not the latter (e.g. #slice is defined on Hash but not Sinatra::IndifferentHash), indicating a condition where the aforementioned footguns may be possible. However, I don't believe there's an easy, portable way to hook onto an "app is finished loading" event.

Member

mwpastore commented Sep 20, 2018

There are some horrifyingly subtle bugs that can happen here if you don't get the code ordering just so. It didn't make sense (to me) to bury the warning in a piece of documentation that no one will ever read. I agree that it is aggressive and ugly, but I disagree that it "creates more issues than it solves."

I'm definitely open to alternative solutions and will defer to the Sinatra team on this issue. An opt-out environment variable is probably the easiest and most portable "workaround," if a bit kludgy. We could only show the warning in dev and/or test. We could print to stderr instead of stdout. Or some combination of these.

A neat solution would be to defer the check until the app is completed booted and then compare the signatures of Hash and Sinatra::IndifferentHash to see if the former has been monkey-patched but not the latter (e.g. #slice is defined on Hash but not Sinatra::IndifferentHash), indicating a condition where the aforementioned footguns may be possible. However, I don't believe there's an easy, portable way to hook onto an "app is finished loading" event.

@DannyBen

This comment has been minimized.

Show comment
Hide comment
@DannyBen

DannyBen Sep 20, 2018

Ok..... just note that for me at least, changing the warning to only show in non production is not going to cut it (unless it is also deferred to actual boot). In my command line utilities that happen to have Sinatra as the driver behind one of their sub commands, this warning will always show - like this:

$ madness --help
WARNING: If you plan to load any of ActiveSupport's core extensions to Hash, be
sure to do so *before* loading Sinatra::Application or Sinatra::Base. If not,
you may disregard this warning.
Madness

Usage:
  madness [PATH] [options]
  madness create config
  madness create theme FOLDER
  madness (-h|--help|--version)

...(truncated)...

DannyBen commented Sep 20, 2018

Ok..... just note that for me at least, changing the warning to only show in non production is not going to cut it (unless it is also deferred to actual boot). In my command line utilities that happen to have Sinatra as the driver behind one of their sub commands, this warning will always show - like this:

$ madness --help
WARNING: If you plan to load any of ActiveSupport's core extensions to Hash, be
sure to do so *before* loading Sinatra::Application or Sinatra::Base. If not,
you may disregard this warning.
Madness

Usage:
  madness [PATH] [options]
  madness create config
  madness create theme FOLDER
  madness (-h|--help|--version)

...(truncated)...
@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 20, 2018

Member

We only need to show the warning message on older Rubies, so maybe that's a step we can take immediately. What version of Ruby are you running under?

Member

mwpastore commented Sep 20, 2018

We only need to show the warning message on older Rubies, so maybe that's a step we can take immediately. What version of Ruby are you running under?

@DannyBen

This comment has been minimized.

Show comment
Hide comment
@DannyBen

DannyBen Sep 20, 2018

We only need to show the warning message on older Rubies

This sounds great. Which is the highest version that requires this warning?
I am personally using 2.5.0, but I develop gems that support back to the area of 2.2.2 usually.

DannyBen commented Sep 20, 2018

We only need to show the warning message on older Rubies

This sounds great. Which is the highest version that requires this warning?
I am personally using 2.5.0, but I develop gems that support back to the area of 2.2.2 usually.

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 20, 2018

Member

Anything under 2.5 could potentially be monkeypatched by ActiveSupport and break IndifferentHash.

Member

mwpastore commented Sep 20, 2018

Anything under 2.5 could potentially be monkeypatched by ActiveSupport and break IndifferentHash.

@sammyhenningsson

This comment has been minimized.

Show comment
Hide comment
@sammyhenningsson

sammyhenningsson Sep 21, 2018

I don't understand how this is supposed to work. My tests are now failing in TravisCI since I don' have activesupport installed. If activesupport is now a required dependency, then please add it to the gemspec!!

sammyhenningsson commented Sep 21, 2018

I don't understand how this is supposed to work. My tests are now failing in TravisCI since I don' have activesupport installed. If activesupport is now a required dependency, then please add it to the gemspec!!

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 21, 2018

Member
Member

mwpastore commented Sep 21, 2018

@sammyhenningsson

This comment has been minimized.

Show comment
Hide comment
@sammyhenningsson

sammyhenningsson Sep 22, 2018

Now that ActiveSupport is used in IndifferentHash I think it should be a dev dependency for Sinatra.
I have a project that does not use neither Sinatra::Contrib nor ActiveSupport. Since 2.0.4, my tests are now failing because Sinatra is trying to load ActiveSupport.

sammyhenningsson commented Sep 22, 2018

Now that ActiveSupport is used in IndifferentHash I think it should be a dev dependency for Sinatra.
I have a project that does not use neither Sinatra::Contrib nor ActiveSupport. Since 2.0.4, my tests are now failing because Sinatra is trying to load ActiveSupport.

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Sep 22, 2018

Member

@sammyhenningsson ActiveSupport is not used by IndifferentHash and it was never intended to be a dependency, but yes, there is a bug in the logic that conditionally requires ActiveSupport (to solve an error in Sinatra's internal testing suite), and it should be resolved by #1477.

EDIT: If you want to know more about the history of this issue, here is a deeper discussion.

Member

mwpastore commented Sep 22, 2018

@sammyhenningsson ActiveSupport is not used by IndifferentHash and it was never intended to be a dependency, but yes, there is a bug in the logic that conditionally requires ActiveSupport (to solve an error in Sinatra's internal testing suite), and it should be resolved by #1477.

EDIT: If you want to know more about the history of this issue, here is a deeper discussion.

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