Simplify configen

[theblixguy]

This PR makes significant changes to configen in order to simplify it. Below is an explanation:

Motivation

There are a couple of reasons that motivated me to rewrite configen:

1. configen takes a lot of time to set up in a project. It's also a bit complicated for a beginner to learn how it works - example: it took me nearly an hour to learn how to set it up for my project at TAB.
2. The use of an external build system seems unnecessary to me when you can add the same command to a pre-action run script to the build step in each scheme.
3. configen takes on a lot of heavy lifting to ensure the variables defined in the plist files correctly map to the variables defined in the mapping file. This is unnecessary work that can actually be delegated to Xcode.
4. When creating the plist and .map file in Xcode, you do not get any real-time feedback on errors or mistakes. You will only get that when building the project (and you'll have to dig into the build log to find the error).
5. You're restricted to Foundation types - you cannot use a non-Foundation type. You also cannot use Optionals - you have to set the value to nil (which gets wrapped in quotes) or 0 or some other value in the plist to get around it.
   • Since only Foundation types are supported, you cannot use, for example, an enum case to specify the OSLogType when calling the os_log function.
   • In some cases, you will need to perform a check on the value of a variable if you set it to nil or 0 or some other value in a particular environment plist file. It would be nicer if you had a true optional variable, so you can make use of if/guard let or optional chaining. This would be better than checking for some hard-coded value.
6. It feels like a compiler (taking code in a source language i.e. plist (xml) + .map (swift-like) & emitting code in a target language i.e. swift/objc) but doesn't do the job in a robust way.

Solution

My solution to the issues is to rewrite configen from scratch and solve the problems in a more simple way:

1. Get rid of the mapping file: Use a Swift protocol to ensure all environment configuration files have the same variables.
2. Get rid of the plist file: Use a Swift struct that conforms to the protocol and defines the values for the variables for each environment.

Using a Swift protocol and struct feels a lot more familiar and easier to work with while offering the necessary safeguards. For example, if the environment configuration struct does not properly conform to the protocol, then an error in the source editor will appear in real-time. Same with if an incorrect value (i.e. value of different type) is assigned to a variable.

By using a protocol, the compiler will enforce, in real-time, that all the environment configuration files have the same variables and that values are assigned to all of them. It is a much better way to ensure consistency across environment configuration files and also allows you to take advantage of Swift features directly (example - you can have truly optional variables).

There is no longer a need to use plist and custom mapping files, when you can take advantage of the language features to achieve the same goal.

1. Re-implement code generation: Instead of having everything hard-coded, a better way is to traverse the AST (Abstract Syntax Tree) of our environment configuration struct and make the necessary changes. This can be done by using the open source SwiftSyntax framework, which is built on top of libSyntax and is used in Xcode's Swift migration tool. This is an official Apple framework, built on top of a library that is used by the compiler and hence is well suited for our needs.

Because of the way the protocol and the struct is binded, it's actually a lot simpler to generate the final output struct. The only changes required is to remove the protocol conformance, rename the struct's identifier (to the user supplied name) and add a private initializer to prevent anyone from accidentally creating an instance of the struct (this is because all the variables are going to be static lets). This can all be done by traversing the AST and making the above transformations. The end result is the same input configuration struct, but slightly altered for use in our app.

Some other small changes include using Swift Package Manager to manage dependencies, making the code a little more Swifty, adding documentation, etc.

Result

It is a lot easier to set up and use configen in a project. The use of protocols makes creating environment configuration files much simpler and provides safeguards to ensure that the files have the same key-value pairs, while also making it easier for the user to create those files (i.e. allowing the user to quickly insert the missing variables into the file using suggestions and code completion and receiving real-time, compiler-enforced feedback on errors and mistakes).

configen is now faster as well because it is no longer performing a lot of unnecessary parsing and code generation. Using a simple pre-action run script in each scheme saves a lot of time as well compared to going through the steps to create an external build system and integrating it with each scheme.

Notes

There are breaking changes (!!!) and as a result, configen is not fully backwards compatible:

• Plist and map files are no longer supported and is replaced by protocols and structs. However, it's relatively simple to migrate from plist/maps to protocol/structs and can be done in a short amount of time. Perhaps I can write a migration tool that transforms the plist+map to protocol and struct.

• Objective-C support has been dropped.

• A small change will be required to the command line arguments, as you only need to pass the input environment configuration file, the name of the output struct and the name of the output folder (only 3 arguments compared to 5 before).

• No changes will be required in the application code (as long as you pass the same output struct name).

---

Please refer to the README and the example project to understand how everything works.

[theblixguy]

@Utsira I think we could remove the *.xcodeproj file (put it in the gitignore), because it will be regenerated when running the SPM commands. People who want the binaries can always download it from the Releases section, so I don't think there will be a need to clone the repo and compile the project (unless you're submitting a change), but yeah, we could definitely have a script that runs all the necessary commands for us.

#!/bin/bash

swift package clean
swift package update
swift package generate-xcodeproj
swift build -c debug -Xswiftc -static-stdlib

[Jon889]

I'm not a TABer but I work with TAB at Tesco which uses Configen in the way described by @samdods here. So it's not the most basic usage of Configen, with multiples modules declaring properties they need to be configured with, and then passing the Config to the modules so they can get the configured values. We also extend the Config type with values that need to be created at runtime, or are common between all environments.

I've just switched the project (on branch chore/configen-simple for those with access) to using your branch of Configen, and while it was pretty easy to switch one plist/map to struct/protocol I don't think I'd want to switch all 13 environments over (even with Xcode 10's multi cursor support) so I think that conversion tool would be needed :) I think this gives decent evidence that your changes can be merged as even the fairly non standard use of configen in our Project was able to switch, also I think your work and this way of configuration is pretty great 😀

I did note some things:

• using enums instead of structs would mean that the private init isn't needed.
• making the environment structs private (so that the problem Ben showed above doesn't happen) makes the final output struct also private. Replacing line 48 of FileRewriter.swift with the code below (or something better, I've not used SwiftSyntax before) fixes it.
• It might be good to enforce that the input structs are private*
• I'm not sure why you want to remove the conformance to the protocol in the final output struct.

    var nodeModifiers = node.modifiers
    if let privateModifierIndex = node.modifiers?.firstIndex(where: { $0.name.tokenKind == TokenKind.privateKeyword }) {
      nodeModifiers = node.modifiers?.removing(childAt: privateModifierIndex)
    }
    return node.withIdentifier(newIdentifier).withInheritanceClause(nil).withMembers(newMembersList).withModifiers(nodeModifiers)

*We recently switched to using the Scheme pre build action way (IIRC, we found Xcode 10 was building the external build system target in parallel with the main app, even after turning off parallelize build). The one problem with that was that you can't fail the build from the pre-action step if Configen fails (eg if enforcing that input structs are private). I got round this by printing an #error() to the output Config.swift in the pre-action build script. eg:

configName=Config
configDir="$SRCROOT/$project/Config"
configen --config-path "$configDir/Settings/Config-$env.swift" --struct-name $configName --output-directory "$configDir" > "$configDir/build-config.log" 2>&1
if [ $? -ne 0 ]; then
  echo -e "#error(\"build-config.sh failed, output is in $configDir/build-config.log\")" > "$configDir/$configName.swift"
fi

[Jon889]

I also went on to make some more modifications to reduce duplication that we had. But it required a few tweaks to your code, to allow the output to be a class instead of a struct. I can go into more detail about why this would be helpful on our project but I don't want to clutter this thread. The summary is that it allows each environment class/struct to conform to a composed protocol ie typealias ConfigProtocol = AppConfig & ModelConfig & ProductConfig // & etc, and that each environment's class can inherit from a base class containing the config properties that are the same across all environments or generated at runtime. This allows each module to say what it needs to be configured, without duplicating it in the the main protocol that each environment implements.

[theblixguy]

Thanks for the feedback @Jon889 :)

• using enums instead of structs would mean that the private init isn't needed.

That's actually a good idea! It would also mean that you would have to use config enums instead of config structs. Or would you prefer structs be converted into an enum, instead of having the user create enums in the first place? We can just use a class as well.

• making the environment structs private (so that the problem Ben showed above doesn't happen) makes the final output struct also private. Replacing line 48 of FileRewriter.swift with the code below (or something better, I've not used SwiftSyntax before) fixes it.

Thanks, I'll fix that!

• It might be good to enforce that the input structs are private*

Hmm, can do!

• I'm not sure why you want to remove the conformance to the protocol in the final output struct.

This is because at that point, the protocol's job is over. Its there to ensure that all config files contain the declared variables and that those variables have values assigned to them (unless optional). Since the protocol guarantees that for us, we can be sure that the generated config file will have the variables we need (and that values will be assigned to them, unless the values are optional).

[theblixguy]

I can go into more detail about why this would be helpful on our project but I don't want to clutter this thread. The summary is that it allows the protocol that each environment class/struct inherits from be composed from multiple protocols ie typealias ConfigProtocol = AppConfig & ModelConfig & ProductConfig // & etc, and that each environment's class can inherit from a base class containing the config properties that are the same across all environments or generated at runtime.

Ah okay, makes sense, I'll make it a class!

[theblixguy]

Thanks for the feedback @bengilroy!

It seems very documentation heavy – do we need all the documentation?

I added a lot of docs to help understand what's going on at a glance (thought it would be helpful for someone new). I guess the documentation is a little more verbose than it should be!

Dropping Obj-C Support

I looked at the TAB Engineering Best Practices google doc and all current projects use Swift. Perhaps there might be projects that are still on Objective-C and use configen, but they could still use the old release of configen and not migrate to the new one.

Seems like the indentation for the private init() on AppConfig uses tabs but the rest use spaces – is this intentional?

Whoops, I should replace that with spaces. The issue would be figuring out 'how many spaces?', but I'll figure out a way!

Where documentation is used, it seems do be divided into public/private using mark comments – but in most cases I saw we weren't actually using public it was defaulting to internal? This is just a minor technicality though.

You're right, I'll fix the MARK comments.

Some doc comments have full stops at the end of sentences, but some don't.

Will fix!

[Jon889]

That's actually a good idea! It would also mean that you would have to use config enums instead of config structs. Or would you prefer structs be converted into an enum, instead of having the user create enums in the first place? We can just use a class as well.

Thanks for the replies :) For simple usage I think enum is the best as an instance can't be created, but class allows more flexibility, so allowing both should be ok. I'm not sure what would be best to do if the inputs are structs, perhaps just accept all three and trust the user has their reasons for using it that way.

This is because at that point, the protocol's job is over. Its there to ensure that all config files contain the declared variables and that those variables have values assigned to them (unless optional). Since the protocol guarantees that for us, we can be sure that the generated config file will have the variables we need (and that values will be assigned to them, unless the values are optional).

I don't think there is any harm in letting the output conform to the protocol, as a minor point it would confirm that configen has done it's job correctly. But really it will be useful when the protocol is composed of other protocols. eg:

typealias ConfigProtocol = ModelConfig & ProductConfig

private class DevConfig: ConfigProtocol {}

class AppConfig: ConfigProtocol {} //Generated by Configen

func setup(with: ModelConfig.Type) {}

setup(with: AppConfig.self)

Without the generated output class also conforming to the protocol it would not be possible to do the final line.

[theblixguy]

For simple usage I think enum is the best as an instance can't be created, but class allows more flexibility, so allowing both should be ok. I'm not sure what would be best to do if the inputs are structs, perhaps just accept all three and trust the user has their reasons for using it that way.

Okay, so basically the output type would be the same as input? So if you created the configs as enums, then the output would also be an enum? This could probably be a feature behind a flag (--match-input-type). I would prefer the output be an enum, so the config file looks cleaner and so people don't accidentally try to create an instance. But for those who want flexibility, they can pass in the additional command-line flag.

I don't think there is any harm in letting the output conform to the protocol, as a minor point it would confirm that configen has done it's job correctly. But really it will be useful when the protocol is composed of other protocols. eg:

That's true, perhaps I can add a flag to configen to keep the inheritance clauses intact (--keep-inheritance-clauses)? Some people may put all the config related files in EXCLUDED_SOURCE_FILE_NAMES in Xcode. I was talking to @KaneCheshire and he mentioned that the project he's working on does not allow the config files to be compiled with the production build, so those files will be added to EXCLUDED_SOURCE_FILE_NAMES in Xcode and thus you will no longer be able to make use of the config protocol, unless you skip the protocol file and put the rest. Also, I think the code would look cleaner without the inheritance clause!

[samdods]

Also, btw, Configen supports any custom types, such as enum (explained in the README), and this also means it supports optionals (if defined as optional in the mapping file) and as far as i remember, the nil doesn't get added in quote (although i need to check)

[samdods]

@Jon889 i don't think you'd want to subclass, you should be able to achieve what you need by conforming to multiple protocols.

[samdods]

i just confirmed optionals are supported as per the README documentation and custom types are supported as per docs and also shown in the example project.

[samdods]

although i think going with optionals kinda goes against the purpose of Configen. the intention is to avoid runtime checks on whether you have the config setup. the project i work on has 30 config parameters and i haven't yet found a need for optionals.

[samdods]

@theblixguy i really commend the effort you've put into rewriting this 👍👍 but would be really good to beat this kind of thing up beforehand in future. i'm just thinking of saving you a lot of effort 😄

one major disadvantage of this new approach is that all the config files are part of the target, even those that won't be used when the project builds. this means that you'll have production URLs and keys released with your test app and vice versa. not a big problem as far as i'm concerned, but it's definitely something that a penetration testing company would pick up on and as such it's something a project stakeholder would likely oppose. so it's best to make sure each of the env files are not part of the target, like so:

i definitely think this is a great solution to the same problem, but i don't think it's necessary to have a command line utility to achieve the result. i think going with an enum would be the nicest solution (although perhaps i'm missing something about what @Jon889 was saying). and i think you can simply copy the necessary file into place, overwriting the actual file that will be built, i.e. the one file that is part of the target. you'll then know at build-time whether or not your config conforms to the requirements.

[theblixguy]

Thanks for the feedback @samdods, really appreciate it 🙂

• I see, not sure why I couldn't get an optional to work before. I just tried it out again and it works! Although you do have to explicitly assign it to nil in the plist file even if it's an optional, otherwise it ends up emitting static let variableName: variableType? = instead of just static let variableName: variableType?. I think it's more to do with the value, because all config files will have the same variables, but perhaps in a certain environment, you have no need for one, so it's just nicer to assign it nothing. A runtime check might still be required in either case if the thing you're passing the value to does not accept nils or garbage values.
• Custom types are supported, but not any type. This is because configen doesn't have a way to figure out where a type comes from, in order to insert the necessary imports. For example, if you use the OSLogType type in the mapping file and provide a value in the plist, the code will successfully be generated, but the app won't compile as the config file will be missing the import for os. You can get around it by creating your own enum as a wrapper around OSLogType and mapping your cases to its cases but that's just extra work. This is just an example, but basically you’re restricted to Foundation types only at the moment.
• The files are a part of the target, however there’s two workarounds for it if you don’t want it compiled - you can remove the files from the target once you’ve set up all your configs (or never include them in the first place, but then you lose on compiler warnings/errors) or you can add them to EXCLUDED_SOURCE_FILE_NAMES. I think either of those should solve the issue?

The main goal is to simplify everything and give the user the freedom to do what they want with the config files, while enforcing some common sense rules. I agree that with this approach, configen’s role has been diminished to simply copying the files over (although it still has to make the necessary transformations, so it’s not a case of simple copy and paste). The feedback I got from Jon suggested I do some of my own rules, such as enforcing that the inputs are private and one of my own ideas was to enforce that all variables are declared as static & let, so I believe there’s still some responsibilities in configen’s shoulders, although not as much as before 🙂

[samdods]

regarding the restriction to Foundation types and your own custom types, that's a good point.

regarding nil values, see my comment above about defeating the point of Configen.

regarding removing the files from the target, you say it loses the benefit of compiler warnings/errors, but it doesn't. you might not have the benefit of checking the code as you write it, but you will still get the compiler warnings/errors at build time, which is the whole point of Configen. but the advantage of removing the test keys from the production app and vice versa massively outweighs getting warnings/errors as you write, given this is a file you'll modify relatively rarely.

but i don't understand the responsibility of Configen as you've rewritten it. it really should be as simple as copying the files over. if you've setup your protocol properly, declaring the requirements for static and { get } properties, then the conforming type for each environment will have to meet these requirements.

i also advise not to make such radical changes to an open source utility that's (hopefully!) used on projects outside of TAB.

it's more than just a breaking change, it's a completely new tool, so i think it would be better suited to a new project.

definitely a good discussion going on here, so i don't want this to come across like i discourage this type of beat up 😄 we might even change our project setup to go with this approach...

but i wouldn't rely on a command line utility to do something i can do in one line of a shell script.

---

as an aside, one of the benefits of Configen is that reading config from a Plist makes it more accessible to non Swift developers, both technical people and non-technical people alike. this might include technical stakeholders that don't wish to review Swift code in order to review environment settings. this is a small benefit and might seem trivial, but it's actually the reason this tool was created in the first place. otherwise i would have taken your approach and copied the files over using a single line of shell script.

[theblixguy]

I see, those are definitely valid points, especially that people would find it easier to read plists rather than swift code and that my changes would have an effect on people outside of TAB who also use configen.

Thanks for all the feedback on this, as I learned some new things 😄 I think I'll close the PR now, unless there's any changes that needs to be made.