Improve code generation to avoid unused variable warnings #3184
Conversation
|
An alternative strategy would be to prefix all local variables with |
|
Thanks for opening a pull request. Either approach is fine. This seems ok at a glance, I will review today. We should not include generated changes in the commit because our release system will handle that, and a changelog would be applied for all gems automatically (though it's a generic one about updating generating code - I think that's ok). |
Right, I remembered that from previous contributions, but I figured you'd maybe want to see the result? I can revert that part whenever you like. |
|
Yeah, perfectly fine for review! Your post said you would generate all gems unless I misunderstood. |
|
I enabled CI for you. It looks like there are failures. |
byroot
force-pushed
the
avoid-unused-variable-warnings
branch
from
c8929db to
5da9c51
Compare
|
Thank you. I think I fixed the problem, and I removed the generated files. |
The Rails test suite had these warnings for ages, which I'd like to fix: ``` aws-sdk-s3/endpoint_provider.rb:22: warning: assigned but unused variable - key aws-sdk-s3/endpoint_provider.rb:24: warning: assigned but unused variable - copy_source aws-sdk-s3/endpoint_provider.rb:23: warning: assigned but unused variable - prefix ``` Ultimately I don't think pre-assigning to variables is really worth it, as the `parameters` argument is a T_STRUCT so access is really fast. But I suspect this was done to make the code generation simpler. In the end this adds a little bit of complexity, but not that much.
byroot
force-pushed
the
avoid-unused-variable-warnings
branch
from
5da9c51 to
78c17a2
Compare
|
Adding some historical context here - Smithy validates endpoint rulesets to ensure that all parameters are used. However, S3 needed an exception to support using endpoint rules for a few complex auth resolution cases (eg access grants), so those unused parameters violate our normal validations. We would expect to see these warnings only in S3. |
The Rails test suite had these warnings for ages, which I'd like to fix:
Ultimately I don't think pre-assigning to variables is really worth it, as the
parametersargument is a T_STRUCT so access is really fast.But I suspect this was done to make the code generation simpler. In the end this adds a little bit of complexity, but not that much.
NB: I only regenerated the aws-sdk-s3 gem to show what the change looks like. But if this change is accepted I can regenerate all gems, and add a CHANGELOG entry.