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

Validate config values for arrays and hashes #898

Merged
merged 1 commit into from Apr 13, 2022
Merged

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Apr 12, 2022

Motivation

Since we write Tapioca config as a YAML file, it's easy to make type errors that will result in unexpected behaviors.

For example, even if this config file looks ok, the msgpack gem will not be turned typed: false:

gem:
  typed_overrides:
    msgpack: false

Because we expect the values inside the typed_overrides hash to be strings rather than booleans.

This actually happened today to one of our users: #895 (comment). Having an error message earlier would have saved both of us some time and confusion.

Implementation

The solution choose here seems a bit hack-ish but we can't rely on Thor expected types for the content of :array and :hash options:

Once we validated all others criteria for options keys and values we add an additional check for arrays and hashes to make sure the values (and keys in the case of a hash) are all of the type String.

Even if rather limited, this works in practice because the only arrays and hashes we take in the config always expect string (iirc?).

I guess we can revisit this later if we add different options but I'm open to better suggestions.

Tests

See automated tests.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar added the enhancement New feature or request label Apr 12, 2022
@Morriar Morriar requested a review from a team April 12, 2022 22:33
@Morriar Morriar self-assigned this Apr 12, 2022
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Nice one, thanks!

@Morriar Morriar merged commit 3ab0f4e into main Apr 13, 2022
@Morriar Morriar deleted the at-check-config-values branch April 13, 2022 14:11
@shopify-shipit shopify-shipit bot temporarily deployed to production May 13, 2022 23:08 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants