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

lodash #42

Merged
merged 4 commits into from
Oct 17, 2016
Merged

lodash #42

merged 4 commits into from
Oct 17, 2016

Conversation

marudor
Copy link
Collaborator

@marudor marudor commented Apr 22, 2016

No description provided.

@ryyppy
Copy link
Contributor

ryyppy commented Apr 23, 2016

Would love to see lodash/fp declarations in there too!
https://github.com/lodash/lodash/wiki/FP-Guide

The auto-currying functions are hard to type, not sure how we tackle this, also I was looking for a way to make function composition with compose type-safe.. if someone has an idea for that, that would be really great!

Of course, I will help wherever I can, but these kind of super-generic function declarations are tough.

@vaukalak
Copy link
Contributor

@marudor thanks a lot for this. I'd like to help you with tests to make it merged. But as it's quite a lot of work to do, and work of different people could overlap, I'd propose the following flow:

1 - every method is tested in an appropriate test file.
2 - before someone starts to write tests, he writes here methods he will cover.
3 - all tests go to marudor:lodash

Does this make sense?

@mirek
Copy link

mirek commented Jul 6, 2016

Considering a fact that flow-typed is in beta, wouldn't it be better to merge definitions as they are from this PR (and others) to speed up the process?

I'm sure people will "cherry-pick-commit" fixes much faster.

At the moment definitions live in different branches which makes it very hard to contribute.

Instead of waiting for perfect version, we could have something to work on, even if it's not confirmed 100% correct.

Later we can add some stability flag or coverage report but for now I think just merging not-100%-covered definitions would be a win for the project.

@jeffmo
Copy link
Contributor

jeffmo commented Jul 6, 2016

@mirek: Only the CLI is in beta -- the definitions are real :)

Moreove, it's important that tests pass always -- otherwise we'll dig ourselves into a corner where only some definitions are high quality and others aren't.

Id say it's ok to submit partial libdefs with intent to come back and fill in more API surface area later, but tests and versioning and such if the submitted surface area are pretty important even now.

@mirek
Copy link

mirek commented Jul 6, 2016

What about creating some sort of "sandbox" directory which mimics "definitions"? The focus should be on minimising size of PRs and making contributions easy.

Without this kind of mechanism what are people doing? They look for things like lodash, etc. they won't find definition for it so they probably go back to reconsidering typescript because those were two alternatives they were looking at in the first place.

Some people will dig deeper and see lodash in PR. Hanging there for 3 months, not much activity. They'll copy paste it to their project. Comment out or fix things that are broken and wait for new version. They won't commit back their changes for sure.

The thing is that if people are able to install incomplete definitions (with --sandbox flag or whatever explicit intention) they will test it via their codebase. This is great leverage to use. If type checking fails against their project they will see exactly what kind of test + fix to write so it passes that single error. Just few lines of code, create PR - no problem.

On the flow-typed side - it's much easier as well, small patches that you can cache and run in your head can be accepted very quickly.

The whole thing will move forward.

@jeffmo
Copy link
Contributor

jeffmo commented Jul 6, 2016

Also worth noting that we aim for quick turnaround on PRs that pass tests . In this case, @marudor had a large number of libdefs that were around before flow-typed...so getting through those takes some time. In the future it's more likely that we'll have a more steady stream of libdefs written with flow-typed in mind (and thus tests + versioning, etc) -- so hopefully those will land more steadily.

We should probably prioritize this particular one higher for turn-around than others tho (given than lodash is pretty popular)

@jeffmo
Copy link
Contributor

jeffmo commented Jul 7, 2016

(sorry, I made the second comment without seeing that you'd already responded)

What about creating some sort of "sandbox" directory which mimics "definitions"?

That's not a bad idea. We could move PRs there after some SLA of non-responsiveness rather than just closing them (this inevitably happens on every GitHub project occasionally). I also like the idea of excluding them from search/install results in the CLI without a special --include-sandbox (or whatever) flag.

I'll make an issue to track this

@BlooJeans
Copy link

@marudor
Copy link
Collaborator Author

marudor commented Oct 12, 2016

I've updated it and reused the underscore tests.
Lets get this merged - finally.

@marudor marudor merged commit 6cadc08 into flow-typed:master Oct 17, 2016
@marudor marudor deleted the lodash branch October 19, 2016 14:40
cullophid pushed a commit to cullophid/flow-typed that referenced this pull request Jun 19, 2017
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

6 participants