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

Chai tweaks #511

Merged
merged 2 commits into from Dec 22, 2016
Merged

Chai tweaks #511

merged 2 commits into from Dec 22, 2016

Conversation

@scottnonnenberg
Copy link
Contributor

@scottnonnenberg scottnonnenberg commented Dec 5, 2016

A couple changes were required to get rid of all Flow warnings on a 6000-line project:

  1. Chai allows you to check length with a function call: .length(4)
  2. Chai's configuration option truncateThreshold is a number, not a boolean
@ryyppy
Copy link
Contributor

@ryyppy ryyppy commented Dec 5, 2016

Alright, in the beginning I was confused by the official lodash#map documentation, since it didn't state anything about string as iteratee... but then I saw the examples which explicitly states

var users = [
  { 'user': 'barney' },
  { 'user': 'fred' }
];
 
// The `_.property` iteratee shorthand.
_.map(users, 'user');
// => ['barney', 'fred']

Rest LGTM as well... I think we can merge this? @marudor

@ryyppy
Copy link
Contributor

@ryyppy ryyppy commented Dec 5, 2016

Dang, this looks quite familiar to #486 as well :(

@ggregoire
Copy link
Contributor

@ggregoire ggregoire commented Dec 8, 2016

Indeed, #486 fixes this case of _.map. I'm going to add some tests as asked by @marudor.

I will probably just copy/paste the examples from the Lodash docs.

@scottnonnenberg
Copy link
Contributor Author

@scottnonnenberg scottnonnenberg commented Dec 8, 2016

Happy to remove the lodash aspects of this PR, just leaving the chai elements. Seems like y'all would like some tests as well? Do you have any other feedback I could address which might help this get approved?

@ryyppy
Copy link
Contributor

@ryyppy ryyppy commented Dec 8, 2016

@scottnonnenberg Yeah, please remove the lodash stuff and add some tests for your chai changes, then I am more than happy to merge... for lodash we will merge #486 to fix the issues :-)

@ggregoire Thanks for chiming as well! 👍

@scottnonnenberg scottnonnenberg changed the title Chai and lodash tweaks Chai tweaks Dec 8, 2016
@ryyppy ryyppy merged commit 60acee2 into flow-typed:master Dec 22, 2016
1 check passed
@ryyppy
Copy link
Contributor

@ryyppy ryyppy commented Dec 22, 2016

Sorry for the delay and thanks for all the work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants