observing / fullcontact

FullContact API bindings for Node.js
MIT License
42 stars 18 forks source link

Cleanup #10

Closed AdriVanHoudt closed 9 years ago

AdriVanHoudt commented 9 years ago

What I still want to do is rework the tests to not use it() for logging stuff, it says now that 18 tests are pending. Or are these tests that need to be made yet? And maybe update the docs to include the webhook stuff.

Also I don't know if mocha can do this but Lab(my go to test lib) can do linting and can even make the tests fail if there are linting errors. And it can be configured to look like mocha. If you want this I'll be happy to replace it.

I will also use this as my base for implementing the company api (sorry I couldn't do that sooner) so we can either merge this or just the full thing later. If you have any remarks please feel free. @observing/full-contact-write

AdriVanHoudt commented 9 years ago

@3rd-Eden what are your thoughts on moving to Lab? And are those empty it()'s tests that need to be made?

3rd-Eden commented 9 years ago

@AdriVanHoudt Not fan of lab and I don't see any benefits for it for this this library. And yes, the empty it's are placeholders for tests that still need to be written.

AdriVanHoudt commented 9 years ago

@3rd-Eden can I ask you why not? and what should those tests include? maybe I can help

AdriVanHoudt commented 9 years ago

@3rd-Eden I'll do a PR with lab implemented and then we can discuss it there, still interested in your opinion on why not to go with lab though

3rd-Eden commented 9 years ago
  1. I have personal preference of not using anything that eran wrote because that guy a pain to deal with. Which makes contributing nearly impossible.
  2. Switching to lab doesn't add any extra benefits compared to using mocha.
  3. All tests under this organization use mocha
  4. The default API of Lab is pretty bad so we would have keep the bdd approach which actually results in writing more code to make it work with lab.
  5. If you want to run linting in addition to testing you can just do change the test script to a multiple script invocations or just use pre-commit to have the linting happen on pre-commit.
  6. You can make a PR with lab, but it will not accepted as it adds nothing to the project.
Swaagie commented 9 years ago

First of all, thanks for the nice contributions, I'd rather not move to lab either. Most of the observing modules are using assume/expect/mocha and I'd like to keep it that way. This reduces external dependencies.

Also I'm wondering what the thoughts behind changing https://github.com/observing/fullcontact/pull/10/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R18 are? I've learned not to trust developers to correctly follow semver ;) and generally tend to be really defensive with it. Minor bumps usually (although they shouldn't) break stuff.

AdriVanHoudt commented 9 years ago

@3rd-Eden

  1. Eran is no longer the lead maintainer of Lab (or almost any Hapi module except Hapi core) and I don't see that as a valid reason but I can see the friction
  2. It has easy build in code coverage and linting options which can indeed be achieved by running multiple scripts
  3. Yeah I'm not to familiar with the organization or its modules but that's a valid point
  4. making lab look like mocha is 5 lines per test file I think that's workable
  5. then I won't, I'm done with it but I'll take it as a good exercise then @Swaagie Keeping consistency inside the org is indeed important. As stated above I'm not familiar with the org. Changing those is mostly readability, the .x.x notation is way more clear than a vague ~ but If you want I'll lock the minor versions for you
AdriVanHoudt commented 9 years ago

I'll make the PR and close it immediately, if you want take a look and otherwise it's there for reference so I don't lose it by accident.