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

Emit warnings when configuration files reference unexpected or deprecated properties #77

Merged
merged 12 commits into from Aug 2, 2022

Conversation

liamnichols
Copy link
Member

@liamnichols liamnichols commented Aug 2, 2022

Background

A while ago, I made a simple typo in my .create-api.yml file and it took me an unexpectedly long time to figure it out. And today, I came across a similar typo in one of the test fixtures so I wanted to do something about it to improve the experience when starting out with the tool 😬

In addition, we need a mechanism to clearly start deprecating preferences in some scenarios.

Changes

In this Pull Request, I build upon the new CreateOptions target and introduce a new warnings property on GenerateOptions. This allows our generator to check the warnings after loading a config and emit either warning or error messages based on the strictness of the generator run.

To do this, there were three big changes that needed to be made:

  1. Make CreateOptions responsible for loading GenerateOptions from a file/data
  2. Introduce an internal IssueRecorder class that can be passed down through Decoder
  3. Wrap KeyedDecodingContainer with a class called StringCodingContainer
    • This allows me to use StringCodingKey which allows container.allKeys to work
    • It keeps a nicer interface in the generated init(from:) implementation
    • It allows us to record which properties were decoded and figure out which values in allKeys remained (i.e typos or unexpected keys)
    • Records issues in the IssueRecorder

With some adjustments to the sorcery generated Decodable implementation, everything plugs together relatively nicely.

After implementing all of this, the GeneratorTests.testGitHub() started failing since it runs in --strict and had a typo in the config file 😄 I fixed this, and updated the snapshots to match what we had intended in the first place.

@liamnichols liamnichols added this to the 0.1.0 milestone Aug 2, 2022
@liamnichols liamnichols self-assigned this Aug 2, 2022
overrideResponses:
overridenResponses:
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that even overridenResponses and overridenBodyTypes suffer from typos as well, but these are already defined and in-use. It should be overriddenX instead.

In a followup PR, I will correct the typos and deprecate the old properties. More on that in a minute

Comment on lines +47 to +48
// Provide an opportunity to make any required mutations
process(&configOptions)
Copy link
Member Author

Choose a reason for hiding this comment

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

This remains since we need to mutate ConfigOptions when loading it, but I don't want to make anything inside GenerateOptions mutable.

Generate.swift

let options = try GenerateOptions(data: data) { options in
    options.entities.include = Set(options.entities.include.map { Template(arguments.entityNameTemplate).substitute($0) })
    options.entities.exclude = Set(options.entities.exclude.map { Template(arguments.entityNameTemplate).substitute($0) })
}

I hope that we can rework this in the future. Maybe #52 might help if we consider moving arguments into GenerateOptions too.

Comment on lines +20 to 24
{% for variable in type.variables|stored %}
{% if variable.annotations.deprecated %}
(.{{ variable.name }}, "{{ variable.annotations.message }}"),
{% endif %}
{% endfor %}
Copy link
Member Author

Choose a reason for hiding this comment

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

It might not be quite clear from this template, especially since we're not using it yet, but this allows us to annotate properties in ConfigOptions.swift as deprecated so that they produce warnings.

For example:

// sourcery: skip, deprecated, message = "Renamed to overriddenResponses"
public var overridenResponses: [String: String] = [:]

The annotations:

  • skip - Omits the property from the documentation
  • deprecated - Causes it to be included in this deprecations array
  • message - Extra details to be printed for the user helping to guide them away from this property

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

1 participant