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

Remove duplicated code from buildASTSchema and extendSchema #1000

Merged
merged 1 commit into from
Dec 2, 2017

Conversation

IvanGoncharov
Copy link
Member

Duplicated code caused issues in the past, for example, #961.
Also, it makes PRs that I working right now significantly bigger.
I tried to leave code intact as much as possible to simplify review.
@wincent Can you please review it?

@IvanGoncharov IvanGoncharov force-pushed the unifyBuildSchema branch 3 times, most recently from c6a538e to 10ad85a Compare August 24, 2017 16:47
@IvanGoncharov
Copy link
Member Author

I did a second pass and unify even more code now both function share type caching logic.
@wincent Would be great to hear your thoughts on overall design and on the naming of the new class and methods?

@wincent
Copy link
Contributor

wincent commented Aug 25, 2017

@IvanGoncharov: I'm going to look at this one now. If your intention is that it should replace #999 please go ahead and close that one. (I haven't actually looked at that one yet.)

@wincent
Copy link
Contributor

wincent commented Aug 25, 2017

This is great. Really good to see the redundancy eliminated here. The naming looks fine to me. I'd suggest doing a sweep through making sure that only methods that need to be public on the ASTDefinitionBuilder class are actually public (at least buildWrappedType, for instance, looks like it could be _buildWrappedType).

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Aug 27, 2017

I'd suggest doing a sweep through making sure that only methods that need to be public on the ASTDefinitionBuilder class are actually public (at least buildWrappedType, for instance, looks like it could be _buildWrappedType).

@wincent Done ✅

I'm going to look at this one now. If your intention is that it should replace #999 please go ahead and close that one.

I'm trying to keep separate PRs for separate changes. So #999 change the same function but intended to simplify different parts of it. After one of the PRs will be merged I will rebase the second one.

@leebyron leebyron merged commit 57ba580 into graphql:master Dec 2, 2017
@IvanGoncharov IvanGoncharov deleted the unifyBuildSchema branch December 14, 2017 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants