making warnings less scary #59

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
6 participants
@alexch
Contributor

alexch commented May 11, 2011

https://rubyforge.org/tracker/index.php?func=detail&aid=29176&group_id=126&atid=575

rubygems still prints out deprecation warnings, but after the first warning, it buffers up all remaining warnings and then emits them at_exit in a somewhat nicer report format. (See the last test in test_deprecate.rb for a sample.)

I also pulled in the "--no-warnings" patch, but set the default back to "show all warnings" (i.e. skip=false).

lib/rubygems/specification.rb
@@ -274,6 +274,9 @@ class Gem::Specification
def self._resort! # :nodoc:
@@all.sort! { |a, b|
+ if a.nil? or b.nil?

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

Why is this code here? It should be removed.

@evanphx

evanphx May 11, 2011

Member

Why is this code here? It should be removed.

This comment has been minimized.

@alexch

alexch May 11, 2011

Contributor

oops

@alexch

alexch May 11, 2011

Contributor

oops

lib/rubygems/specification.rb
@@ -547,7 +550,7 @@ class Gem::Specification
rescue SignalException, SystemExit
raise
rescue SyntaxError, Exception => e
- warn "Invalid gemspec in [#{file}]: #{e}"
+ warn "Invalid gemspec in [#{file}]: #{e}\n\t#{e.backtrace.first}"

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

Remove the backtrace stuff.

@evanphx

evanphx May 11, 2011

Member

Remove the backtrace stuff.

lib/rubygems.rb
@@ -120,7 +121,7 @@ require "rubygems/deprecate"
# -The RubyGems Team
module Gem
- VERSION = '1.8.1'
+ VERSION = '1.8.2'

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

Remove this. Only the release manager gets to change the version.

@evanphx

evanphx May 11, 2011

Member

Remove this. Only the release manager gets to change the version.

This comment has been minimized.

@alexch

alexch May 11, 2011

Contributor

oops

@alexch

alexch May 11, 2011

Contributor

oops

lib/rubygems/deprecate.rb
@@ -21,25 +21,76 @@
# end
module Deprecate
+
+ SKIP_DEFAULT = false

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

What is the point of this constant? I only see it used to have it set to false.

@evanphx

evanphx May 11, 2011

Member

What is the point of this constant? I only see it used to have it set to false.

This comment has been minimized.

@alexch

alexch May 11, 2011

Contributor

So if someone wants to rebuild the gem with the default set to true, it'll be dead easy for them. This was a compromise with tadman's patch
https://gist.github.com/965921 which set skip to true by default, but the logic was weird so I extracted it and put in a test so it works right no matter what the default value is. Since the warnings can occur any time the rubygems library is used, we needed some global mechanism to set the user's preference. Rebuilding the gem is clearly inferior to, e.g., a config file, but much easier to code.

@alexch

alexch May 11, 2011

Contributor

So if someone wants to rebuild the gem with the default set to true, it'll be dead easy for them. This was a compromise with tadman's patch
https://gist.github.com/965921 which set skip to true by default, but the logic was weird so I extracted it and put in a test so it works right no matter what the default value is. Since the warnings can occur any time the rubygems library is used, we needed some global mechanism to set the user's preference. Rebuilding the gem is clearly inferior to, e.g., a config file, but much easier to code.

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

What is "the gem" in this case? I can't imagine you mean that an individual gem would use const_set to change this.

@evanphx

evanphx May 11, 2011

Member

What is "the gem" in this case? I can't imagine you mean that an individual gem would use const_set to change this.

This comment has been minimized.

@alexch

alexch May 11, 2011

Contributor

The outcry to the deprecation spam (which I prefer to call "warningasm") is so great that some people (e.g. tadman) are resorting to building and installing their own version of RubyGems, so "the gem" is rubygems-update, or what you get from "rake gem" in the rubygems source dir.

@alexch

alexch May 11, 2011

Contributor

The outcry to the deprecation spam (which I prefer to call "warningasm") is so great that some people (e.g. tadman) are resorting to building and installing their own version of RubyGems, so "the gem" is rubygems-update, or what you get from "rake gem" in the rubygems source dir.

This comment has been minimized.

@alexch

alexch May 12, 2011

Contributor

...note that this dubious feature is a minor aspect of this patch, and I'd be happy to separate it. I pulled in tadman's patch since he'd already written a test for Deprecate so I could get started coding right away.

@alexch

alexch May 12, 2011

Contributor

...note that this dubious feature is a minor aspect of this patch, and I'd be happy to separate it. I pulled in tadman's patch since he'd already written a test for Deprecate so I could get started coding right away.

This comment has been minimized.

@evanphx

evanphx May 12, 2011

Member

If a person goes all that distance, I think they should have to figure out how skip works and change the false to a true in the method.

@evanphx

evanphx May 12, 2011

Member

If a person goes all that distance, I think they should have to figure out how skip works and change the false to a true in the method.

This comment has been minimized.

@alexch

alexch May 18, 2011

Contributor

OK, code has been re-obfuscated :-)

@alexch

alexch May 18, 2011

Contributor

OK, code has been re-obfuscated :-)

lib/rubygems/deprecate.rb
yield
ensure
Deprecate.skip = original
end
+ require 'ostruct'

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

Adding other dep to rubygems is a no-no. This should go and Warning should just be a normal object.

@evanphx

evanphx May 11, 2011

Member

Adding other dep to rubygems is a no-no. This should go and Warning should just be a normal object.

This comment has been minimized.

@alexch

alexch May 11, 2011

Contributor

Roger that. Though I thought ostruct was a standard library... is it not in all distros?

@alexch

alexch May 11, 2011

Contributor

Roger that. Though I thought ostruct was a standard library... is it not in all distros?

This comment has been minimized.

@evanphx

evanphx May 11, 2011

Member

It is, but that doesn't matter. There is no reason for ostruct to be used here when a normal object will do. It just adds another unnecessary dep to rubygems.

@evanphx

evanphx May 11, 2011

Member

It is, but that doesn't matter. There is no reason for ostruct to be used here when a normal object will do. It just adds another unnecessary dep to rubygems.

@alexch

This comment has been minimized.

Show comment
Hide comment
@alexch

alexch May 12, 2011

Contributor

Made Evan's fixes. Also made the report more concise (it puts all lines from the same file together, e.g. "foo.rb:12,15,345")

Contributor

alexch commented May 12, 2011

Made Evan's fixes. Also made the report more concise (it puts all lines from the same file together, e.g. "foo.rb:12,15,345")

@WilliamDenniss

This comment has been minimized.

Show comment
Hide comment
@WilliamDenniss

WilliamDenniss May 25, 2011

+1 to merge this…

+1 to merge this…

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Jun 2, 2011

Member

alexch, thanks for the fixes. We're still discussing this a little bit, hopefully should get it figured out today.

Member

evanphx commented Jun 2, 2011

alexch, thanks for the fixes. We're still discussing this a little bit, hopefully should get it figured out today.

@tadman

This comment has been minimized.

Show comment
Hide comment
@tadman

tadman Jun 2, 2011

This also includes a unit test for Deprecate, now Gem::Deprecate, which is worryingly not present in the master branch.

tadman commented Jun 2, 2011

This also includes a unit test for Deprecate, now Gem::Deprecate, which is worryingly not present in the master branch.

@alexch

This comment has been minimized.

Show comment
Hide comment
@alexch

alexch Jun 7, 2011

Contributor

It looks like a bunch of the warning spam has been removed as of RubyGems 1.8.5, especially thanks to
21cccd5
and
fe1367c

I still think this patch is worthwhile, mostly because it has unit tests for the Deprecate module (as tadman pointed out and first added). If you'd like me to rebase to master and bring it up to snuff, I'd be happy to, but if you're going to pass on it, that's cool too; please let me know your decision either way.

Contributor

alexch commented Jun 7, 2011

It looks like a bunch of the warning spam has been removed as of RubyGems 1.8.5, especially thanks to
21cccd5
and
fe1367c

I still think this patch is worthwhile, mostly because it has unit tests for the Deprecate module (as tadman pointed out and first added). If you'd like me to rebase to master and bring it up to snuff, I'd be happy to, but if you're going to pass on it, that's cool too; please let me know your decision either way.

@tadman

This comment has been minimized.

Show comment
Hide comment
@tadman

tadman Jun 7, 2011

I have another patch which is desperately required for 1.8.5: #70

I am getting slammed with add_specs deprecation warnings because of Bundler 1.0.14. This turns a sea of warnings into a quiet little grumble.

If it's practical, that should be merged in with this by whomever is undertaking the next build. I would have supplied a patch for the unit test, too, but that is still not in the main branch and I can't patch against nothing.

tadman commented Jun 7, 2011

I have another patch which is desperately required for 1.8.5: #70

I am getting slammed with add_specs deprecation warnings because of Bundler 1.0.14. This turns a sea of warnings into a quiet little grumble.

If it's practical, that should be merged in with this by whomever is undertaking the next build. I would have supplied a patch for the unit test, too, but that is still not in the main branch and I can't patch against nothing.

@ghost ghost assigned zenspider Jun 7, 2011

@alexch

This comment has been minimized.

Show comment
Hide comment
@alexch

alexch Jun 8, 2011

Contributor

It looks like the majority of the warning noise has been suppressed as of Rubygems 1.8.5, which is great! However, the possibility of multiple (redundant, noisy, easily ignored) warnings popping up later (cf. tadman's pull 70) means there's still an underlying flaw in the way rubygems reports warnings. This patch would close that door.

Contributor

alexch commented Jun 8, 2011

It looks like the majority of the warning noise has been suppressed as of Rubygems 1.8.5, which is great! However, the possibility of multiple (redundant, noisy, easily ignored) warnings popping up later (cf. tadman's pull 70) means there's still an underlying flaw in the way rubygems reports warnings. This patch would close that door.

@alexch

This comment has been minimized.

Show comment
Hide comment
@alexch

alexch Jun 8, 2011

Contributor

Related bug ported to github: #84

Contributor

alexch commented Jun 8, 2011

Related bug ported to github: #84

@practicingruby

This comment has been minimized.

Show comment
Hide comment
@practicingruby

practicingruby Aug 25, 2011

Contributor

Hi @alexch,

Finally got some answers about this. It doesn't look like this patch is going to get merged, because the RubyGems maintainers have been burned in the past by removing deprecated APIs that they didn't warn people in a sufficiently noisy way about. I know this is a controversial decision, but with the case-by-case reduction in the amount of warnings being outputted, this is increasingly becoming a corner case issue for those working with legacy applications.

As for the tests you added to Deprecate, those won't be completely lost in the shuffle. This will be worked on by @zenspider, and I've created ticket #167 to track this issue.

I am now closing this ticket, but thanks for working on this. Even if the team is going another direction, I appreciate that you were trying to help out with this problem.

Contributor

practicingruby commented Aug 25, 2011

Hi @alexch,

Finally got some answers about this. It doesn't look like this patch is going to get merged, because the RubyGems maintainers have been burned in the past by removing deprecated APIs that they didn't warn people in a sufficiently noisy way about. I know this is a controversial decision, but with the case-by-case reduction in the amount of warnings being outputted, this is increasingly becoming a corner case issue for those working with legacy applications.

As for the tests you added to Deprecate, those won't be completely lost in the shuffle. This will be worked on by @zenspider, and I've created ticket #167 to track this issue.

I am now closing this ticket, but thanks for working on this. Even if the team is going another direction, I appreciate that you were trying to help out with this problem.

@alexch

This comment has been minimized.

Show comment
Hide comment
@alexch

alexch Aug 25, 2011

Contributor

Sigh. I wish they would see that they're way past "sufficiently" and into "counterproductively" noisy, but whatever.

Thanks for being so responsive and great, @sandal!

Contributor

alexch commented Aug 25, 2011

Sigh. I wish they would see that they're way past "sufficiently" and into "counterproductively" noisy, but whatever.

Thanks for being so responsive and great, @sandal!

@practicingruby

This comment has been minimized.

Show comment
Hide comment
@practicingruby

practicingruby Aug 25, 2011

Contributor

@alexch: I'll address the point about noisiness over on #84, I'm about to close that as well.

Contributor

practicingruby commented Aug 25, 2011

@alexch: I'll address the point about noisiness over on #84, I'm about to close that as well.

nobu pushed a commit to nobu/rubygems that referenced this pull request Jan 12, 2014

Merge pull request #59 from enricosada/Time_strftime_N
added directive '%N' to format string of Time#strftime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment