Skip to content

Conversation

@jonathanglasmeyer
Copy link
Contributor

As discussed in #172, i've implemented the resolver option defaultAttributes.

I also fixed the flakyness issue in resolver.test.js by not asserting on the order of the compared arrays in two cases.

I wasn't entirely sure about some code style issues that are not answered by the .eslintrc:

  • var foo = require('...') vsimport ...`
  • const vs. var?
  • arrow fn callbacks vs. function()?
  • use of es2015/16 features like destructuring -- which ones are "allowed" / which node version is targeted?

It might be a good idea to provide more strict guidance per .eslintrcfor a more homogenous code look and feel -- I spent the majority of time thinking about those issues and how to "fit in" to the existing code style(s). :)

@mickhansen
Copy link
Owner

import
const/let over var
arrow when you need the context, otherwise function
whatever es6 you like, we use babel

you're welcome to update .eslintrc

@jonathanglasmeyer
Copy link
Contributor Author

Ok, great, I'll add some rules to .eslintrc then. I'll also create a small CONTRIBUTING.md guide with some setup & general advice if you don't mind, just as a starting point. I'll do this in another PR though, this one is finished from my side.

@mickhansen
Copy link
Owner

LGTM, i would appreciate a squash to a few more descriptive commits.

@jonathanglasmeyer
Copy link
Contributor Author

Will do.

(1)
.eslintrc: Ignore testing globals

Mocha globals like `it` & `describe` trigger eslint warnings because
they are not imported explicitly. Setting them as `globals` in
.eslintrc fixes that.

(2)
Make eslintrc compatible with chai assertion expressions

There is no way to make eslint shut up about chai's assertion
expressions (`expect(foo).to.be.true`) without disabling this rule.

See eslint/eslint#2102

(3)
Lint resolver.test.js
There were two flaky tests, both related to assertions on specific
orderings of arrays. One case expected the order of `user.task` to be
deterministic which is not the case, the other expected the order of
the graphql schemas `users` field response to be deterministic.

The issues were fixed by just asserting that members of the compared
arrays were deep equal.
@jonathanglasmeyer
Copy link
Contributor Author

I squashed it.

mickhansen added a commit that referenced this pull request Mar 18, 2016
Enhancement: Resolver option `defaultAttributes`
@mickhansen mickhansen merged commit f997e28 into mickhansen:master Mar 18, 2016
@mickhansen
Copy link
Owner

v0.22.0

@jonathanglasmeyer jonathanglasmeyer deleted the pr/default-attributes branch March 18, 2016 09:30
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.

2 participants