Skip to content

Conversation

@ruudk
Copy link
Collaborator

@ruudk ruudk commented Oct 28, 2021

Instead of checking for array or callable, why not check for iterable instead.

This makes it possible to use yield to provide types.

@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #993 (33f3cf6) into master (4754e67) will not change coverage.
The diff coverage is 50.00%.

❗ Current head 33f3cf6 differs from pull request most recent head 59d5d66. Consider uploading reports for the commit 59d5d66 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master     #993   +/-   ##
=========================================
  Coverage     94.46%   94.46%           
  Complexity       50       50           
=========================================
  Files           118      118           
  Lines          9610     9610           
=========================================
  Hits           9078     9078           
  Misses          532      532           
Impacted Files Coverage Δ
src/Type/Definition/FieldDefinition.php 90.90% <50.00%> (ø)
src/Type/Definition/InputObjectType.php 94.82% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4754e67...59d5d66. Read the comment docs.

@spawnia
Copy link
Collaborator

spawnia commented Oct 28, 2021

I agree with the direction of this change. Please fix CI, then request a final review from me.

Instead of checking for `array` or `callable`, why not check for `iterable` intead.

This makes it possible to use `yield` to provide types.
@ruudk
Copy link
Collaborator Author

ruudk commented Oct 28, 2021

@spawnia Ready.

@spawnia spawnia changed the title Allow types to be returned using yield Allow field definitions to be defined as any iterable, not just array Oct 28, 2021
@spawnia spawnia merged commit a40f1e1 into webonyx:master Oct 28, 2021
@spawnia
Copy link
Collaborator

spawnia commented Oct 28, 2021

Nicely done, thank you

@ruudk ruudk deleted the allow-yields branch October 29, 2021 11:40
@ruudk
Copy link
Collaborator Author

ruudk commented Oct 29, 2021

@spawnia Thanks for the merge. Could this be tagged 🙏 ?

@ruudk
Copy link
Collaborator Author

ruudk commented Oct 29, 2021

I think I should have targetted this to v14 instead :( Is it possible to merge it there?

@spawnia
Copy link
Collaborator

spawnia commented Oct 29, 2021

You can add another PR to backport this to 14.x

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