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

Assorted refactorings #404

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Conversation

nightscape
Copy link
Contributor

I'm trying to understand the code a little better and applying small refactorings here and there.
I'm grouping them by theme, so that changes you don't like can be taken out.
Any comments welcome 😃

Copy link
Member

@OlegIlyenko OlegIlyenko left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on the PR! In general I find it quite good 👍

I would suggest to avoid changes in TypeInfo and OverlappingFieldsCanBeMerged, but I think we can keep changes everywhere else. Both of these places are very often used so they might have big performance impact. Also OverlappingFieldsCanBeMerged needs to be kept in sync with reference implementation very closely (and it is the the most complex and expensive validation of them all).

WDYT?

@travisbrown
Copy link
Contributor

@nightscape Would you be interested in migrating this PR to the new GitHub organisation?

@nickhudkins nickhudkins force-pushed the refactorings branch 2 times, most recently from 4652784 to 9e8f6f3 Compare December 10, 2020 01:05
@nickhudkins
Copy link
Contributor

Hey @nightscape I've rebased and broken out the commits that Oleg had mentioned being worried about (with regards to performance).

My inclination is to drop the commits with [WARNING] in them unless we had some sort of benchmark to know if it has impact, even though my gut says that the changes don't scream performance issues.

I'm gonna go ahead and leave this PR open for now as is, and if I haven't heard back by early next week I'll go ahead and split the WARNING commits into their own PR.

@nightscape
Copy link
Contributor Author

Hi @nickhudkins, thanks for taking care!
I wouldn't expect performance issues either (rather the other way around as the Scala collections might have optimized implementations), but your proposal about splitting into two PRs sounds like a good approach.

@nickhudkins
Copy link
Contributor

@nightscape split :)

Copy link
Contributor

@yanns yanns left a comment

Choose a reason for hiding this comment

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

👍

@yanns yanns merged commit a671cf6 into sangria-graphql:master Dec 11, 2020
@nightscape nightscape deleted the refactorings branch December 11, 2020 09:28
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.

5 participants