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
Single source of truth for configuration options #52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liamnichols
commented
Jul 2, 2022
liamnichols
force-pushed
the
ln/options-and-documentation
branch
3 times, most recently
from
July 26, 2022 21:56
da2e9e0
to
02b3205
Compare
…documentation and default values
…custom Decodable implementation (using default values)
…t with documentation and default values
…ormance and regenerate documentation
liamnichols
force-pushed
the
ln/options-and-documentation
branch
from
July 30, 2022 13:49
c4a5185
to
2ad7afe
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Links
Background
As it stands, the GenerateOptions.swift file contains a few things:
GenerateOptionsSchema
class that are all optional allowing initialisation from an empty yaml/json fileGenerateOptions
class and initialisers that map from the schema type with a default value (+ some additional customisations)In addition to that, we have an example config file in the README document.
I've identified this as a bit problematic and hard to maintain because we have two types to represent the same thing, the documentation is maintained manually (and a little out of sync) and the default values are also decoupled.
I want to explore better ways to capture schema of the options with one single source of truth (that contains the properties, their defaults, and documentation).
Approach
In this change, I'm reintroducing a new
ConfigOptions
type inside a new CreateOptions target (this is because it will also later be shared with the generator plugin target). This time, there are some differences: There is now just one single type where every property is defined, documented and has a default value.Now there are reasons why we didn't do this before:
Decodable
ignores the default values defined for each property so an empty config would fail to deserialiseBut this is where I've chosen to use Sourcery to help out.
Sourcery will generate two things:
Decodable.init(from:)
implementation that will fallback to the default value if it wasn't provided in the YAML/JSONThe caveat here is that whenever we modify ConfigOptions.swift, we have to re-run Sourcery (
make documentation
), but the upside there is that one type now acts as the single source of truth for everything.Usually I'd probably have given up and just dealt with the manual maintenance of options/docs/mapping, but since CreateAPI has a lot of flexibility and a rich set of options, I think it's important for this to be accurate.
I then have a small wrapper around
ConfigOptions
that is calledGenerateOptions
(this will replace the old one). This wrapper currently just provides the small additional functionality of computingallAcronyms
but could also be used in the future to abstract loading the config as well as tracking the file location in the event that we want to put file paths in the config files (so we can easily support relative paths).In this PR, I've not started using the new
GenerateOptions
yet, that will come in a different PR. I'll also update the README in another PR too.While the PR is open, find the documentation here: https://github.com/CreateAPI/CreateAPI/tree/ln/options-and-documentation/Docs/ConfigOptions.md
I'd love any feedback on what can be improved with the format 🙏