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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn some models from TuistCore that are classes into structs #870

Merged
merged 6 commits into from Jan 8, 2020

Conversation

@pepibumur
Copy link
Contributor

pepibumur commented Jan 7, 2020

Short description 馃摑

As we start optimising things by introducing parallelisation, mutations of models might result in race conditions or crashes that impact the stability perceived by the users.

Solution 馃摝

This PR discourages and prevents mutation of the models in TuistCore by turning them into structs that we pass around. Moreover, it also extracts the scheme-specific structs and enums into their own files to make them more searchable. Some examples of those elements are Arguments, TestableReference, or TestAction.

Mutating properties of a model instance will be a code smell and Xcode will warn developers that attempt to do that.

@pepibumur pepibumur self-assigned this Jan 7, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Jan 7, 2020

@pepibumur your pull request is missing a changelog!

@pepibumur pepibumur requested review from lakpa and tuist/core Jan 7, 2020
@pull-assigner pull-assigner bot requested review from kwridan and ollieatkinson and removed request for tuist/core Jan 7, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #870 into master will increase coverage by 0.33%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #870      +/-   ##
==========================================
+ Coverage    78.5%   78.84%   +0.33%     
==========================================
  Files         183      199      +16     
  Lines       10874    10796      -78     
==========================================
- Hits         8537     8512      -25     
+ Misses       2337     2284      -53
Impacted Files Coverage 螖
Sources/ProjectDescription/Scheme.swift 100% <酶> (+13.15%) 猬嗭笍
Sources/TuistCore/Models/Settings.swift 98.36% <酶> (-0.08%) 猬囷笍
Sources/TuistCore/Models/InfoPlist.swift 62.71% <酶> (-11.94%) 猬囷笍
Sources/TuistCore/Models/Scheme.swift 100% <酶> (+37.06%) 猬嗭笍
Sources/TuistCore/Models/TuistConfig.swift 57.14% <酶> (+17.14%) 猬嗭笍
Sources/TuistCore/Models/Workspace.swift 75% <酶> (+7.25%) 猬嗭笍
Sources/TuistCore/Models/Headers.swift 100% <酶> (酶) 猬嗭笍
Sources/TuistCore/Models/LintingIssue.swift 100% <酶> (酶) 猬嗭笍
Sources/TuistCore/Models/CoreDataModel.swift 100% <酶> (+50%) 猬嗭笍
Sources/ProjectDescription/ArchiveAction.swift 0% <0%> (酶)
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update b401c76...118fed6. Read the comment docs.

@lakpa
lakpa approved these changes Jan 7, 2020
public let preActions: [ExecutionAction]
public let postActions: [ExecutionAction]

public init(targets: [TestableTarget] = [],

This comment has been minimized.

Copy link
@lakpa

lakpa Jan 7, 2020

Contributor

only difference between these two initialisers are one takes configurationName and other config, we can wrap these two parameters under an enum.

This comment has been minimized.

Copy link
@pepibumur

pepibumur Jan 8, 2020

Author Contributor

That's a good suggestion, although I wouldn't do it as part of this PR.


// MARK: - Equatable
public static func == (lhs: ArchiveAction, rhs: ArchiveAction) -> Bool {

This comment has been minimized.

Copy link
@lakpa

lakpa Jan 7, 2020

Contributor

All of ArchiveAction properties auto conform to equatable, the manual equatable will not be needed

This comment has been minimized.

Copy link
@pepibumur

pepibumur Jan 8, 2020

Author Contributor

Right. I went through all the structs and deleted the manual conformance of the protocol. The only one that I couldn't remove was the Target model, because some attributes are tuples and they don't conform the equatable. In a follow-up PR I might turn those into structs instead.

@pepibumur pepibumur merged commit 3dc88e5 into master Jan 8, 2020
20 checks passed
20 checks passed
Swiftlint Swiftlint
Details
Swiftlint Swiftlint
Details
Unit tests
Details
Unit tests
Details
Build
Details
Build
Details
Changelog
Details
Changelog
Details
Release build
Details
Release build
Details
Features
Details
Features
Details
Upload
Details
Upload
Details
Header rules Deploy canceled
Details
Mixed content Deploy canceled
Details
Pages changed Deploy canceled
Details
Redirect rules Deploy canceled
Details
codecov/patch 93.33% of diff hit (target 78.5%)
Details
codecov/project 78.84% (+0.33%) compared to b401c76
Details
@pepibumur pepibumur deleted the classes-to-structs branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.