Add RBS/Steep type-declarations and checking #28
Conversation
This was behaving correctly before, but only because Array(nil) is [], which might surprise some people (as it did me).
The behavior was already correct (and tested as such), since nil =~ _ returns nil, but this skips the method and is easier to follow.
This is just to satisfy the type-checker
instead, put it in the Gemfile, and allow an ENV to exclude it, which we can then supply in the rspec github actions. We need to run tests in environments that steep doesn't support, and gemspecs don't (as yet) support optional dev dependencies.
|
@soutaro - if you have a moment, I'd love a glance to make sure I'm using this right in the general sense (so that someone using this gem would have the advantage of the type signatures). Please don't spend your time on a substantial code-review, of course :-) |
| @@ -1,3 +1,7 @@ | |||
| source "https://rubygems.org" | |||
|
|
|||
| gemspec | |||
|
|
|||
| unless ENV["NO_STEEP"] | |||
There was a problem hiding this comment.
Using group would be better for this but I'm not sure if there is any specific reason it doesn't work...
There was a problem hiding this comment.
No, that should work just fine. It didn't occur to me because I've only ever used them coupled to rails environments -.-
There was a problem hiding this comment.
well.. Actually I can't seem to make it work, but only in github actions on the 2.7 container. Setting the BUNDLE_WITHOUT=typing environment variable suffices in all of the other slices of the matrix here to skip installing steep, but on 2.7 I still get the error message
Because steep >= 1.6.0.pre.1 depends on Ruby >= 3.0.0
and Gemfile depends on steep ~> 1.6.0,
Ruby >= 3.0.0 is required.
It's quite irritating, because that works fine locally on the same combination of ruby/bundler versions. In the action it also seems to ignore being supplied --without directly (despite it being deprecated) and my installing them via bundle config set --local without 'typing' && bundle install.
If what I'm doing wrong is obvious to you, please de-frustrate me, but otherwise I think I'll just leave it - I'm actually just supplying a different environment variable and using that to control the group-selection, so in the end it's not really improving anything, just being a little clearer about intentions.
|
|
||
| module AccessHelpers | ||
| # (Actually provided by ENV, which these are all extended onto) | ||
| def fetch: (String name) -> String? |
There was a problem hiding this comment.
You may want to use an optional parameter, as (String name, ?String? default) -> String?. But they are equivalent in this case.
There was a problem hiding this comment.
Yes, that's definitely more appropriate, thanks :-)
| library "date" | ||
| end | ||
|
|
||
| target :test do |
There was a problem hiding this comment.
Looks like there is no test directory in this repo?
Supporting rspec is difficult. So, I don't think having Steep enabled with specs doesn't make sense...
There was a problem hiding this comment.
Goodness, that's embarrassing! Never came back to update this after I understood how the config worked -.-
|
I grouped the updates matching your suggestions (aside from the group one, at which I've failed) into #32, thanks for taking the time :-) |
Add a single .rbs file, and install Steep to statically check the types. Add a github workflow that runs
steep checkas an action on-push to enforce that they stay up to date. (passing run here, failing run here)A few minor changes were required to satisfy the type-checker (though none affected actual behavior).
And then, awkwardly, we need to run specs on ruby 2.7, which steep doesn't support. We don't need steep to run on ruby 2.7, but that means we can't include it in the gemspec dev-dependencies, so we do a little Gemfile/ENV dance to make sure it's installed normally (locally and in CI), but not in the rspec contexts.
(This will resolve #21)