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

Add env option to required-fields rule config #75

Conversation

PepperTeasdale
Copy link
Contributor

@PepperTeasdale PepperTeasdale commented Jun 1, 2017

@jnwng 👀 please
Allows .graphql and .gql literal files to be linted with the required-fields rule. Resolves #74

Note: Neglected to update the README when I removed a few broken rules from the literal env and just threw the update in here, since I was already at it.

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

@PepperTeasdale PepperTeasdale force-pushed the add-required-fields-rule-to-literal-env branch from a407f71 to 934fad4 Compare June 1, 2017 22:59
@troch
Copy link

troch commented Jun 4, 2017

I'm also interested by this change. I've tried @PepperTeasdale change, and it works as expected.

@jnwng
Copy link
Contributor

jnwng commented Jun 27, 2017

wow, apologies for taking so long to get this. this looks good — i was wondering if you could expand a little on the case around supporting graphql/required-fields using the literal env, do you mind adding a test case that verifies that that works as expected? it'd be nice to add that to make sure we don't regress over this particular issue

@PepperTeasdale
Copy link
Contributor Author

No problem @jnwng . My use case is that our clients are writing queries in .graphql files and we want to make sure they always include the required id fields on our types when defined. I can make a separate test case, for sure. I don't really see where in the test suite we are testing .graphql files, so I could use a little guidance on that. I updated the existing test for this rule to supply the apollo env, but could easily make a separate test case which does not supply the env option, which would validate that we are not breaking the current behavior. Please advise on what you would like.

@PepperTeasdale PepperTeasdale force-pushed the add-required-fields-rule-to-literal-env branch from 934fad4 to e99980d Compare July 12, 2017 00:38
Allows .graphql and .gql literal files to be linted with the required-fields rule.
@PepperTeasdale PepperTeasdale force-pushed the add-required-fields-rule-to-literal-env branch from e99980d to 6160158 Compare July 12, 2017 00:41
@PepperTeasdale
Copy link
Contributor Author

@jnwng 💇‍♂️ I went ahead and added a test case to verify that my change didn't cause a regression. It still doesn't add any tests that the literal .graphql files are covered (again, just unsure what is desired, maybe some kind of integration test?).

@jnwng
Copy link
Contributor

jnwng commented Jul 18, 2017

@PepperTeasdale i didn't have an answer to your question about how to deal with testing the processing of .graphql files (as they fairly obviously work with this library already). i wanted to have a better answer to give you and attempted to take a stab at it, but its still not 100% there (you can see my code here: 198f160) the crux of the matter is that we cannot use the normal RuleTester and need to pass in some configuration to allow us to use ESLint processors in our tests.

i'd like to give this one more day for me (or yourself, if you can get these tests running) before merging — if the code that i wrote can parse and validate rules properly, i'd like to have them both go in at the same time. if its not straightforward, i'll merge this and open an issue to make sure we provide some guidance on making sure we're not regressing in our literal env.

does that sound reasonable?

@jnwng
Copy link
Contributor

jnwng commented Jul 18, 2017

nevermind i got the tests running. PTAL @PepperTeasdale!

describe('processors', () => {
it('should define processors', () => {
function execute(file) {
const cli = new CLIEngine({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥 I was looking for something just like this, but didn't know this existed.

@PepperTeasdale
Copy link
Contributor Author

@jnwng That's awesome! If you think it's worth it, I can start another branch and take a crack at backfilling some other tests in the literal env (I believe people consider this the best practice for writing queries, so would be nice to add more coverage, IMO). LGTM!

@jnwng
Copy link
Contributor

jnwng commented Jul 18, 2017

@PepperTeasdale that would be awesome! thanks so much for working with me on this, and apologies again for taking so long to turn everything around. will merge and release once CI passes

@jnwng jnwng merged commit 17c39d5 into apollographql:master Jul 18, 2017
@stubailo
Copy link
Contributor

Awesome!

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

4 participants