Fix warning on using Channel.select

[asterite]

#8409 (comment)

[jkthorne]

should there be a test for this?

[bcardiff]

If deprecated code is not tested it is not guaranteed to stay working.

I prefer a CI with warnings rather than untested deprecated methods. But If I'm the only one who thinks that way, we can go ahead.

[asterite]

If deprecated code is not tested it is not guaranteed to stay working

Oh, I didn't know that was the reason it was kept.

[straight-shoota]

Maybe we could consider adding comments for such specs that are explicitly meant to test deprecated behaviour. Otherwise I expect we will eventually get more "fix warning" PRs in the future.

[straight-shoota]

I'll award bonus points for having a mechanism for designating a spec as testing a specific deprecated method and disabling deprecation warnings for this method inside the spec's body.

[bcardiff]

Definitely. But is either be

• a new keyword to be applied in a given lexical scope (as checked in [WIP] Arithmetic with Overflow #6223) or
• an annotation that could express only for the whole method level. (not useful in specs)
• magic comments (I don't like it)

So, something like this?

pragma(allow_deprecated: true) { expr }

[asterite]

I don't think this is worth our efforts. After all it's just a warning.

[straight-shoota]

I agree. It was just an idea that popped into my head but should be filed under "in case you're bored and have nothing else to do".

[straight-shoota]

I don't know why I kept thinking about this. But it bugged me. And I think this could actually be very simple to implement. Because this really only affects specs and deprecated methods should only be called directly in the spec body, a naive static filter should be enough.

require "spec"

@[Deprecated]
def deprecated
end

it "test deprecated method" do
  deprecated
end

This is the warning:

In deprecation_spec.cr:8:3

 8 | deprecated
     ^---------
Warning: Deprecated top-level deprecated.

crystal spec, after compiling the program reports the warnings. These could simply be filtered based on file and line number. If it's in a spec file and maybe has some kind of annotation naming the deprecated method that should be suppressed.

Obviously this won't work if the deprecated method is called indirectly. But that should be avoided anyway because it would mean the deprecated method is used in some code other than specs.

[asterite]

The warnings are reported during compilation, not at runtime. I don't know if you took that into account.

[straight-shoota]

Yes. Runtime doesn't matter. The spec command knows about the warnings an has the source code. So it can easily lookup the warning location in the source and check whether there is for example a comment designating this spec as testing a deprecated method.

[RX14]

The warnings code already has a check for which directories to ignore warnings in (lib). It would be trivial to expose that on the crystal commandline, and add spec.

[straight-shoota]

This off-topic discussion should be continued in #8477