chore: add type signatures and type checker for CI #58
Conversation
Signed-off-by: Manuel Schönlaub <[email protected]>
Signed-off-by: Manuel Schönlaub <[email protected]>
|
Hey, @maxveldink any chance this could get approved. It would bring the RBS in a somewhat usable state. |
|
Also tagging @josecolella to get a feel for who maintains this now. |
|
Howdy 👋🏻 I'll give this a look tonight and make sure I have notifications properly set up for this repo. |
Gemfile.lock
Outdated
There was a problem hiding this comment.
This probably shouldn't be removed. Were you running into an issue with it present?
There was a problem hiding this comment.
Hm... I think I had a bundler version mismatch and then remebered that it wasn't recommended to check Gemfile.lock in anyways. However, this recommendation ahs since been revised and so yeah, the lockfile is back :)
| @@ -26,7 +26,7 @@ def initialize(name:, version: nil) | |||
| end | |||
|
|
|||
| def ==(other) | |||
| raise ArgumentError("Expected comparison to be between Metadata object") unless other.is_a?(Metadata) | |||
| raise ArgumentError, "Expected comparison to be between Metadata object" unless other.is_a?(Metadata) | |||
There was a problem hiding this comment.
This seems unrelated to the RBS changes. Did we pick up a new Rubocop rule?
There was a problem hiding this comment.
Nope, it's actually steep complaining.
lib/openfeature/sdk/metadata.rb:29:14: [error] Type `::OpenFeature::SDK::Metadata` does not have method `ArgumentError`
│ Diagnostic ID: Ruby::NoMethod
│
└ raise ArgumentError("Expected comparison to be between Metadata object") unless other.is_a?(Metadata)
~~~~~~~~~~~~~
Looking at the documentation for Kernel::raise it kind of makes sense. TIL, they are pretty clear about how thet expect raise to be used when setting an error_message.
| @@ -43,6 +43,17 @@ jobs: | |||
| - run: bundle install | |||
| - name: Rubocop | |||
| run: bundle exec rubocop --color | |||
| steep: | |||
There was a problem hiding this comment.
@josecolella @technicalpickles Thoughts on enforcing this check on CI if we're going to start supporting RBS?
Signed-off-by: Manuel Schönlaub <[email protected]>
|
It's worth noting the initial rbs was just part of the @josecolella and I talked about sorbet for typing over on one earlier PRs, ie #8 . Aside from the runtime dependency, I'm in favor of holding off typing until the implementation is a bit more solid. I argued it take more time keeping the types up to date while we are still working out the details, and that that would discourage refactoring.. _:pear:'d with @josecolella on 👀 _ |
|
Closing for #66 |
This PR
Related Issues
Fixes #53
Notes
Some of the types around contexts are probably a bit off. Then again, parts of the spec aren't actually implemented anyways. It seems to be a chicken-egg problem.
Follow-up Tasks
Decide which route to go for the context types (
Hashvsclass). The latter oen ahs better typing support,Hasheswith optional keys are a bit weird.How to test
This comes with a new job in the CI.