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

Ignore Boolean props in require-default-prop rule #529

Merged
merged 3 commits into from
Jul 30, 2018

Conversation

michalsnik
Copy link
Member

This PR addresses #222 by not requiring default key for Boolean props.

I thought about considering the following cases:

{
  a: Boolean,
  b: [Boolean],
  c: {
    type: Boolean,
  },
  d: {
    type: [Boolean],
  }
}

But I'm no longer sure we need to support cases with arrays that contain only Boolean type, and perhaps we should always require default key, when prop type is described by an ArrayExpression 🤔 WDYT @chrisvfritz ?

@michalsnik michalsnik self-assigned this Jul 21, 2018
@michalsnik michalsnik requested a review from chrisvfritz July 21, 2018 20:48
@chrisvfritz
Copy link
Contributor

While b: [Boolean], is a little strange, it's technically acceptable, so I think it's a good case to cover. I could imagine legitimate reasons for preferring it too. For example, some teams may have a convention of always using the array syntax so that it's clear that multiple types can be added. Or, some prop definitions might be defined programmatically from a schema and these might always use arrays.

@michalsnik
Copy link
Member Author

Oh, now I see I didn't cover d actually. I'll update it @chrisvfritz

@chrisvfritz
Copy link
Contributor

Thanks! 🙂

Copy link
Member

@znck znck left a comment

Choose a reason for hiding this comment

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

LGTM.

@michalsnik michalsnik merged commit 766b637 into master Jul 30, 2018
@michalsnik michalsnik deleted the ignore-default-for-bool-prop branch July 30, 2018 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants