Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

static var to static let #2642

Merged

Conversation

michalsrutek
Copy link
Contributor

Summary

  • static let plays nicely with modern Swift concurrency (async/await)
  • static let makes it impossible to mutate the values - causing undefined behavior in some cases

@@ -4834,7 +4834,7 @@
);
mainGroup = B657A8DD1CA646EB00121384;
packageReferences = (
427692E12B98B82500F24321 /* XCLocalSwiftPackageReference "Sources/PushServer/SharedPush" */,
427692E12B98B82500F24321 /* XCLocalSwiftPackageReference "SharedPush" */,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got these changes just by running the project 🤷

@michalsrutek michalsrutek changed the title static var to static let static var to static let Mar 14, 2024
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 28.53%. Comparing base (6c34210) to head (1d6f368).
Report is 52 commits behind head on master.

❗ Current head 1d6f368 differs from pull request most recent head 4b4848c. Consider uploading reports for the commit 4b4848c to get more accurate results

Files Patch % Lines
Sources/App/Scenes/WindowSizeObserver.swift 50.00% 1 Missing ⚠️
...urces/Shared/Common/ObjectMapperTransformers.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2642      +/-   ##
==========================================
+ Coverage   27.54%   28.53%   +0.98%     
==========================================
  Files         311      335      +24     
  Lines       31699    23624    -8075     
==========================================
- Hits         8733     6741    -1992     
+ Misses      22966    16883    -6083     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bgoncal
Copy link
Member

bgoncal commented Mar 18, 2024

@zacwest I see some var becoming let inside structs, thats what I also default using even though struct behavior already assures it would be immutable, I know you have an opinion about this, wanna jump in?

@bgoncal bgoncal requested a review from zacwest March 18, 2024 10:07
Copy link
Member

@zacwest zacwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the most part the changes seem good. I don't think the 'static var…' to 'static let…' cases are a problem.

Keep in mind: this project uses the 'World' pattern from Pointfree, so a lot of mutability is actually meant to be in Current rather than elsewhere, and that's the point where things should be overridden. So for example, static let shared xyz = … should actually generally be in Current rather than a singleton on the type. Makes testing easier.

Comment on lines -4 to -5
public var serverIdentifier: Identifier<Server>
public var settingsKey: String
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cases like this - where it's a data type - shouldn't force the non-static members to be let - that's already done by the mutability state of the struct. It's not in and of itself wrong to copy a struct and mutate the value for some reason, as long as the values are valid state changes -- which the type can guard if it wants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no real impact now, I will merge this PR as-is, we can always change it to var on demand of course when needed to actually do something with the struct.

@bgoncal bgoncal merged commit 7de47c6 into home-assistant:master Mar 25, 2024
4 checks passed
@bgoncal
Copy link
Member

bgoncal commented Mar 25, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants