Erlang 19 dialyzer fixes for S3 #305
Conversation
| @@ -42,7 +42,7 @@ | |||
| secret_access_key::string()|undefined|false, | |||
| security_token=undefined::string()|undefined, | |||
| %% epoch seconds when temporary credentials will expire | |||
| expiration :: pos_integer(), | |||
| expiration=undefined :: pos_integer()|undefined, | |||
There was a problem hiding this comment.
i wonder why it's defaulted only here. is this necessary?
i guess we default it to undefined everywhere or nowhere, agree?
There was a problem hiding this comment.
It's not necessary; I did it because it's in a block of others that all have an explicit default value and security_token right above this already had an explicit-undefined default.
I think that most of the values that get an implicit undefined do that from laziness, but some actually mean it. For example, all the response-related values below this in the record are definitely undefined until the response comes back and they get filled in.
My preference would be to make defaults explicit, because then x=undefined::integer()|undefined means "this can actually be undefined (because it's filled in conditionally, or there's a special meaning when it's not defined)" and x::integer()|undefined means "this is lazy-code and we don't actually know if x needs to accept a value of undefined.
Odds of someone carefully refactoring the whole code-base to eliminate unnecessary implicit undefineds are real low, though.
There was a problem hiding this comment.
I agree with you that non explicit setting is more laziness and just making dializer happy without making code cleaner and thus also prefer explicit setting.
let's leave it here as is.
There was a problem hiding this comment.
I have no strong feeling on this. As long as the specs correctly say it can be undefined, knowing that this is the default value for records where the field is not set seems fine. Explicitly calling out the default is helpful but doesn't seem needed to me.
|
@nalundgaard any feedback? |
| Request3 = erlcloud_retry:request(Config, Request2, fun s3_result_fun/1), | ||
| erlcloud_aws:request_to_return(Request3). | ||
| {RequestHeaders2, RequestBody} = case Method of | ||
| M when M =:= get orelse M =:= head orelse M =:= delete -> |
There was a problem hiding this comment.
any reason not to use ; over orelse?
There was a problem hiding this comment.
it was there before. this is just code move.
|
I think I am 👍 on the change. I asked a few questions above, but neither is really an issue. |
|
okay...i hear no serious arguments so 👍 |
Dialyzer no longer adds an implicit
|undefinedto types inside records, so it rejects most of erlcloud.I'd like to do this in multiple PRs, one per module (except for tiny modules) so that it doesn't feel hopeless to review. If you'd rather one mega-PR, I can do that instead.
This takes care of
erlcloud_s3(starting here because this is the module that bit me), though the header file changes solve a large number of R19 dialyzer complaints across the board.