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

Support none defaultSettings #395

Merged
merged 4 commits into from Jun 8, 2019
Merged

Support none defaultSettings #395

merged 4 commits into from Jun 8, 2019

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Jun 7, 2019

Short description πŸ“

Pairing with @RomainBoulay on porting a project to Swift, we figured out that there's no way to disable the generation of build settings by Tuist.

Solution πŸ“¦

I'm adding a new DefaultSettings case, .none, to disable the default build settings generation.

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

  • Add DefaultSettings.none.
  • Add tests.
  • Update documentation.
  • Update CHANGELOG.

@pepicrft pepicrft requested a review from a team June 7, 2019 01:52
@pepicrft pepicrft self-assigned this Jun 7, 2019
@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #395 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   92.19%   92.22%   +0.03%     
==========================================
  Files         291      291              
  Lines       14615    14673      +58     
==========================================
+ Hits        13474    13532      +58     
  Misses       1141     1141
Impacted Files Coverage Ξ”
Sources/TuistGenerator/Models/Settings.swift 100% <ΓΈ> (ΓΈ) ⬆️
Tests/TuistGeneratorTests/Models/TargetTests.swift 98.88% <100%> (ΓΈ) ⬆️
...uistGeneratorTests/Utils/SettingsHelperTests.swift 100% <100%> (ΓΈ) ⬆️
...stGenerator/Settings/DefaultSettingsProvider.swift 100% <100%> (ΓΈ) ⬆️
...rTests/Settings/DefaultSettingsProviderTests.swift 100% <100%> (ΓΈ) ⬆️

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 41c36d5...3bc640d. Read the comment docs.

@pepicrft
Copy link
Contributor Author

pepicrft commented Jun 7, 2019

@marciniwanicki / @kwridan I have the feeling there's already a way to achieve this but I couldn't find anything.

@tuistbot
Copy link
Contributor

tuistbot commented Jun 7, 2019

1 Warning
⚠️ Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

Generated by 🚫 Danger

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.

yikes, looks like we're missing exposing .none in the ProjectDescription

]}/>

<Message info title="Essential Settings" description="The `.essential` option can be used in the event all warnings are defined in an `xcconfig` file to prevent Tuist from overriding them at the project or target level."/>
<Message info title="Essential Settings" description="The .essential option can be used in the event all warnings are defined in an `xcconfig` file to prevent Tuist from overriding them at the project or target level."/>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth adding a warning style comment for the .none case highlighting Tusit is delegating the responsibility of settings to the user and the project may fail to compile! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this look?

Projects may fail to compile if some essential settings are missing. Use .none only if you are specifying all the necessary build settings manually or via xcconfig files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this @kwridan ?

image

@kwridan
Copy link
Collaborator

kwridan commented Jun 7, 2019

Pushed an update to expose .none.

There are still some settings that get generated by Tuist - all the ones within https://github.com/tuist/tuist/blob/master/Sources/TuistGenerator/Generator/ConfigGenerator.swift#L156 - those are essential to Tuist's functionality (e.g. ensuring Test hosts are setup etc...) - perhaps to allow user control over some of those, we can change the order of precedence

  • Set defaults
  • Apply target derived settings
  • Apply custom user specified settings

@ollieatkinson ollieatkinson added this to the 0.16.0 milestone Jun 7, 2019
@pepicrft
Copy link
Contributor Author

pepicrft commented Jun 7, 2019

those are essential to Tuist's functionality (e.g. ensuring Test hosts are setup etc...) - perhaps to allow user control over some of those, we can change the order of precedence

Isn't that the current order? I think it makes sense to keep Tuist's derived settings because we infer those from the manifest. We could allow the user to control them but if things don't work, they'll blame Tuist for that.

@pepicrft pepicrft merged commit a920a13 into master Jun 8, 2019
@pepicrft pepicrft deleted the empty-default-settings branch June 8, 2019 02:52
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.

None yet

4 participants