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.

Sign up for GitHub

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

Jump to bottom

WIP [cC]onfig => [cC]onfiguration #60

Closed
wants to merge 3 commits into from
Closed

WIP [cC]onfig => [cC]onfiguration #60

wants to merge 3 commits into from

Conversation

toshi0383
Copy link
Collaborator

@toshi0383 toshi0383 commented Sep 21, 2017 •
edited
Loading

  • Replace config to configuration. (Let me know if I'm going too far.😅)
  • Add deprecation message for old value usage.

Related: #59

yonaskolb
yonaskolb reviewed Sep 21, 2017
View reviewed changes
Sources/XcodeGenKit/SettingsPresetFile.swift
case platform(Platform)
case product(PBXProductType)
case productPlatform(PBXProductType,Platform)
case base

var path: String {
switch self {
case let .config(config): return "Configs/\(config.rawValue)"
case let .configuration(configuration): return "Configurations/\(configuration.rawValue)"
Copy link
Owner

Choose a reason for hiding this comment

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

This would break unless the presets config folder was renamed

yonaskolb
yonaskolb reviewed Sep 21, 2017
View reviewed changes
CHANGELOG.md
@@ -87,7 +87,7 @@
- Added `carthageBuildPath` option [PR#34](https://github.com/yonaskolb/XcodeGen/pull/34)

### Fixed
- Fixed installations of XcodeGen not applying build setting presets for configs, products, and platforms, due to missing resources
Copy link
Owner

Choose a reason for hiding this comment

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

references in the changelog shouldn't really be changed

@yonaskolb
Copy link
Owner

Oh wow, you literally renamed every single instance of config in the project :)
I must say, I think I prefer config in code. It's just much easier to read and type.

At the moment in terms of user facing properties in the spec we have following:

  • project.configs
  • project.configFiles (not added yet)
  • target.configs
  • target.configFiles
  • settings.configs (in project, target and setting groups)

These should be the only things that change (as well as the documentation for them).

In addition, I'm actually still on the fence about this change. Does anyone else here have any opinions?

@toshi0383
Copy link
Collaborator Author

IMO config is very troublesome. New user might think configFiles is related to config. Xcode or xcodebuild does not use config for shorthand name for Configuration. I think it's more user friendly to avoid potential confusion with xcconfig. Some user even don't know what Configuration is either, so we should be very very kind as possible.

Usage: xcodebuild [-project <projectname>] [[-target <targetname>]...|-alltargets] [-configuration <configurationname>] [-arch <architecture>]... [-sdk [<sdkname>|<sdkpath>]] [-showBuildSettings] [<buildsetting>=<value>]... [<buildaction>]...

@toshi0383
Copy link
Collaborator Author

I'm not planning to work on this one for now. (I got used to the code-base and I feel good.👌)

@toshi0383 toshi0383 closed this Sep 29, 2017
@toshi0383 toshi0383 deleted the config-configuration branch September 29, 2017 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yonaskolb yonaskolb yonaskolb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@toshi0383 @yonaskolb