Fixed sorbet type strict error. #12761
Conversation
randhircs
force-pushed
the
randhircs/sorbet-fix-to-fix-community-pr-issue
branch
from
299be00 to
1103afd
Compare
kbukum1
force-pushed
the
randhircs/sorbet-fix-to-fix-community-pr-issue
branch
from
1103afd to
d24f751
Compare
| @@ -86,41 +98,50 @@ def create_file_fetcher(directory: nil) | |||
| args = { | |||
| source: job.source.clone.tap { |s| s.directory = directory_to_use }, | |||
| credentials: credentials, | |||
| options: job.experiments | |||
| options: T.unsafe(job.experiments) | |||
There was a problem hiding this comment.
There appears to be a mismatch with the return type of job.experiments. We should create an investigation issue to track this.
Currently, job.experiments returns a hash with symbolized keys (Hash[Symbol, String]), but FileFetcher::Base expects its options parameter to be of type Hash[String, String], which leads to a type incompatibility.
Before taking action, we should confirm whether passing experiments to the file fetcher is actually causing any runtime issues. Once that's clear, we’ll need to figure out how to reconcile the Sorbet types—especially since both FileFetcher::Base and Job are used across all ecosystems and shared code.
There was a problem hiding this comment.
@kbukum1 We are able to transformed the values from Symbol to String and in that way it makes sure that there will not be any runtime type check error. So we may not need to monitor it.
There was a problem hiding this comment.
Can we solve the transforming keys in another PR? It is not related to this PR??
There was a problem hiding this comment.
I feel this PR is ok have this transforming because we are adding sorbet fix and instead of adding unsafe but to transform it.
There was a problem hiding this comment.
I think we should remove that from here. Because in case we got error from this, we want to only revert the transforming, not the whole PR. Also PR is not mentioning or explaining what is solving in this situation. Also do you have a business logic error that you can refer why you are doing this change?
| # prefer credentials directly from the root of the file (will contain secrets) but if not specified, fall back to | ||
| # the job's credentials-metadata that has no secrets | ||
| # Convert experiments hash to have string keys and string values | ||
| experiments_options = job.experiments.transform_keys(&:to_s).transform_values(&:to_s) |
There was a problem hiding this comment.
This should be removed, because it is not related to sorbet typings and it is change for business logic. It should be handled separately. You can find related comment here: #12761 (comment)
… He wants it to add it in separate PR. For this PR I am using T.unsafe to filter sorbet type check.
What are you trying to accomplish?
We have a community PR to pass sorbet related spec we need to add Sorbet type strict for this file in updater.
Anything you want to highlight for special attention from reviewers?
It should pass all test cases.
How will you know you've accomplished your goal?
Checklist