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

Deprecate default check #626

Merged
merged 2 commits into from
Nov 15, 2019
Merged

Conversation

marcandre
Copy link
Contributor

This issues a deprecation warning if one specifies an incompatible default value without either calling allow_incompatible_default_type! or passing check_default_type: false.

See discussion in #621

Copy link
Contributor

@Choms Choms left a comment

Choose a reason for hiding this comment

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

left you a couple comments, rest of the code looks good 👍

raise ArgumentError, err
elsif @check_default_type == nil
Thor.deprecation_warning "#{err}.\n" +
'This will be rejected in the future unless you explicitly pass the options `check_default_type: false`.'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rephrase that as:
'This will be rejected in the future unless you explicitly pass the options `check_default_type: false` or define `allow_incompatible_default_type!` in your code'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above, but "or callallow_..."

it "allows incompatible types if `check_default_type: false` is given" do
expect do
Class.new(Thor) do
allow_incompatible_default_type!
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line so the second test makes sense (else it's equivalent to the previous one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, indeed. And it revealed a bug that was already there whereby an explicit setting was always overwritten by a merge being inverted. Fixed

This reverts commit a88ea56792c9a40d803f025ffcb63e8ae35bf5ff.
@rafaelfranca rafaelfranca added this to the 1.0.0 milestone Nov 15, 2019
rafaelfranca added a commit that referenced this pull request Nov 15, 2019
@rafaelfranca rafaelfranca merged commit 7d199c0 into rails:master Nov 15, 2019
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.

3 participants