Skip to content
This repository was archived by the owner on Sep 2, 2020. It is now read-only.

Go to definition for named types #233

Closed
wants to merge 11 commits into from
Closed

Go to definition for named types #233

wants to merge 11 commits into from

Conversation

divyenduz
Copy link
Contributor

@divyenduz divyenduz commented May 30, 2018

This PR does the following:-

  1. Implement goto definition (with some test cases) for named types with current ability to jump to input, type, enum definition

  2. Add more test cases to getDiagnostics interface

  3. Updates prettier to fix the build

  4. Add ExecutableDefinitions to ignored rules for schema validation

Please let me know what do we need to get this merged. Thanks!

@divyenduz
Copy link
Contributor Author

The build fails but the commands:-

yarn test/node resources/pretty.js --check work for me with the correct status code.

Also, yarn run pretty does not change the code for me.

Might me a build issue?

@divyenduz
Copy link
Contributor Author

I noticed that this build also fails with a similar issue.

@divyenduz
Copy link
Contributor Author

Observed that it was an issue with prettier with node v10. Bumping the prettier version to the latest one fixed the build for me.

Please let me know what do we need to do to get this merged.

@schickling
Copy link
Contributor

schickling commented Jun 1, 2018

@lostplan @mgadda @asiandrummer @wincent would be great if you could quickly review, merge & release this 🚀

@asiandrummer
Copy link
Contributor

@divyenduz Sorry about the delays. It looks like your summary describes 4 different fixes/features, each of which could be divided in its own pull request I think. In the future (and I would even encourage this now, it's not too late!) I would greatly appreciate doing so for following reasons:

  • It makes the reviews much easier
  • If anything breaks, smaller commits allow for a quick bisect to the root cause

It'll take me a bit more time to fully review this - but I'm getting to it :) I appreciate your patience.

@divyenduz
Copy link
Contributor Author

@asiandrummer : Thanks for the feedback. I completely agree with you, so, here are the divided PRs

  1. Feature go to definition SDL - input, enum, type Feature go to definition SDL - input, enum, type #237

  2. Fix: add ExecutableDefinitions to ignored rules for validation fix: add ExecutableDefinitions to ignored rules for validation #234

  3. Fix build: bump prettier Fix build: bump prettier #235

  4. Improve diagnostics tests Improve diagnostics tests #236

Build of 3 PRs will break until #235 is merged that fixes the build.

Please let me know anything further changes needed to get this merged. I am excited to work with you on getting these merged :)

I am keeping this PR open (just in case) but feel free to close it as you like.

Thanks!

@asiandrummer
Copy link
Contributor

Thanks @divyenduz! I'm closing this for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants