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

Make WorkspaceSettings initializer public #658

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

jakeatoms
Copy link
Contributor

@jakeatoms jakeatoms commented Dec 13, 2021

Resolves #657

Short description πŸ“

This PR exposes the initializer of WorkspaceSettings, so we can use it in Tuist. It also makes the autoCreateSchemes a non-optional boolean.

Solution πŸ“¦

I'm working on this Tuist issue: tuist/tuist#3487
The gist is that when a project specifies its own schemes, Xcode begins to nag when the workspace is opened. I'm working on the related Tuist PR, but before I can proceed, I need to be able to initialize this type.
I also think that optional booleans are a source of confusion as their behavior is ambiguous. Making it required will provide a clear behavior to the consumer and have a concrete result.

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

  • Mad the initializer public.
  • Tested this change locally using my local changes to Tuist and successfully generated the file.

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #658 (0165e69) into main (8998757) will increase coverage by 0.08%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
+ Coverage   84.66%   84.75%   +0.08%     
==========================================
  Files         154      157       +3     
  Lines        8878     8987     +109     
==========================================
+ Hits         7517     7617     +100     
- Misses       1361     1370       +9     
Impacted Files Coverage Ξ”
Sources/XcodeProj/Project/Xcode.swift 100.00% <ΓΈ> (ΓΈ)
...ests/Objects/Project/PBXProjIntegrationTests.swift 100.00% <ΓΈ> (+1.29%) ⬆️
.../XcodeProj/Scheme/XCScheme+TestableReference.swift 72.88% <62.50%> (-1.63%) ⬇️
Sources/XcodeProj/Scheme/XCSchemeManagement.swift 87.69% <87.69%> (ΓΈ)
...s/XcodeProjTests/Extensions/XCTestCase+Shell.swift 92.85% <92.85%> (ΓΈ)
Sources/XcodeProj/Project/WorkspaceSettings.swift 95.65% <100.00%> (ΓΈ)
...codeProjTests/Project/WorkspaceSettingsTests.swift 100.00% <100.00%> (ΓΈ)
...codeProjTests/Scheme/XCSchemeManagementTests.swift 100.00% <100.00%> (ΓΈ)
Sources/XcodeProj/Utils/JSONDecoding.swift 72.58% <0.00%> (+3.22%) ⬆️

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 5fd7622...0165e69. Read the comment docs.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this @jakeatoms !

This PR exposes the initializer of WorkspaceSettings, so we can use it in Tuist.

πŸ‘

It also makes the autoCreateSchemes a non-optional boolean.

This is sadly a breaking change 😞 - I'm curious as to the motivation for this change?

Within XcodeProj nil is a valid value and implies not writing the corresponding attributes to the underlying files and letting Xcode pick an appropriate default.

Taking a quick look at the .xcsettings files created by Xcode, the IDEWorkspaceSharedSettings_AutocreateContextsIfNeeded property is indeed optional, as Xcode will not include this by default. As such XcodeProj needs to be able to handle cases where it's missing and also ensure it can omit writing this property too.

Thinking ahead for the changes in Tuist, we'll need to make sure to only create the .xcsettings file and update the corresponding properties when needed (i.e. when we need to explicitly disable the default behaviour), otherwise we'd skip writing this file / or property all together.

/// Path to workspace DerivedData directory.
public var derivedDataCustomLocation: String?

/// When true, Xcode auto-creates schemes in the project.
public var autoCreateSchemes: Bool?
public var autoCreateSchemes: Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is sadly a breaking change 😞

@jakeatoms
Copy link
Contributor Author

jakeatoms commented Dec 14, 2021

I'm curious as to the motivation for this change?

My goal was a better user API, but I was mostly focused on just the property I'm implementing in Tuist. The Descripter would be optional, so if the user didn't set it, it would not create the file. However, your point is well taken because if we implement additional properties in the future, we don't want to write this property as well, if the user wants the default behavior.

That leads me to think that my Tuist implementation of this should be with an enum that defaults to default and maps appropriately to a nil value here. Thanks for the feedback!

Comment on lines +77 to +78
let buildSystem = BuildSystem(rawValue: buildSystemString)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This formatting we done by the Rake task

@jakeatoms jakeatoms changed the title Require WorkspaceSettings.autoCreateSchemes and make initializer public Mak WorkspaceSettings initializer public Dec 14, 2021
@jakeatoms jakeatoms changed the title Mak WorkspaceSettings initializer public Make WorkspaceSettings initializer public Dec 14, 2021
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks @jakeatoms

@pepicrft pepicrft merged commit fae1867 into tuist:main Dec 16, 2021
@pepicrft
Copy link
Contributor

@all-contributors add @jakeatoms for code

@allcontributors
Copy link
Contributor

@pepicrft

I've put up a pull request to add @jakeatoms! πŸŽ‰

@pepicrft
Copy link
Contributor

@jakeatoms I've released a new version of XcodeProj that includes your changes, 8.7.1

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

Successfully merging this pull request may close these issues.

WorkspaceSettings can't be instantiated by consumers
4 participants