search

davetron5000 / gli

Make awesome command-line applications the easy way
http://davetron5000.github.io/gli
Apache License 2.0
1.26k stars 102 forks source link

Fix warnings #248

Closed fornellas closed 2 years ago

fornellas commented 8 years ago

Fixes for warnings reported at. All tests are still passing, and we get no more warnings executing;

ruby -I $PWD/lib -w -rgli -e ''
davetron5000 commented 8 years ago

Hey, so sorry to take forever looking at this. Most of this is :+1: but I have a few questions on some things.

randycoulman commented 7 years ago

I'd really like to see this get merged and released. Is there anything I can do to help get it there?

davetron5000 commented 7 years ago

There are several changes in here I'm not comfortable merging. I'll call them out if it's helpful.

randycoulman commented 7 years ago

Looks like the biggest one has to do with the ERB template stuff, right? Are you OK with the Cleanroom approach that @fornellas is suggesting?

I don't think I can commit changes directly onto this PR. @fornellas Do you have time to address this, or should I make a new PR based on your work here?

davetron5000 commented 7 years ago

OK, commented on each change.

davetron5000 commented 7 years ago

I'm not opposed to the CleanRoom approach, but it seems awfully complex for no actual gain. I will admit that I don't understand the use case that's driving this change. So, from my perspective, lateral changes are fine, but added complexity to satisfy a warning flag isn't something I'm keen to have to maintain.

That said, if @fornellas wants to remove that part of this PR, I can merge, or if someone else wants to make a new PR with just those changes, we can discuss a clean room approach in a different PR.

randycoulman commented 7 years ago

I understand your concerns. As I understand it, the goal of this PR is to be able to run with ruby -w and have clean output. That's certainly why I'd like to see this addressed.

In my projects that use GLI (and a number of other gems), I get a bunch of noisy output from these warnings when running my tests. It makes it really hard to separate the wheat from the chaff.

One could certainly argue whether Ruby's warnings are valid in this case, but the warnings are coming from Ruby itself, not some third-party linting tool. There doesn't appear to be a way to tell Ruby to not warn about certain things when we know they're OK in our context.

I'll give @fornellas some time to respond; otherwise, I'll make a new PR with these changes and a separate one for the cleanroom.

fornellas commented 7 years ago

@davetron5000, do you have another proposal to solve the ERB warning cases? To my eye, the issue is "warnings", and it should solve all of them. I can find the time to work another solution for ERB warnings, but I'd be happy to receive some suggestions as well.

davetron5000 commented 7 years ago

@fornellas can you remove the ERB changes from this PR so I can merge the other stuff? Then we can work on the ERB warnings in another way. Introduce a ton of ivars is not worth it just to remove warnings, but the other changes you made are good and I'd like to merge them.

randycoulman commented 7 years ago

FYI... When I was looking at this stuff, I found a really helpful blog post: http://mislav.net/2011/06/ruby-verbose-mode/. It has really good suggestions on how to work around some of these warnings when the solution is not obvious. Doesn't have anything about the ERB case, though.

fornellas commented 2 years ago

This fell through the cracks, I'm closing this now as no point in reviving this years old PR