Skip to content

Conversation

@hihuz
Copy link
Contributor

@hihuz hihuz commented Jan 10, 2020

Hello 👋

First of all, thank you very much for your work on this library, it has helped us a lot in rapidly creating a graphql endpoint to go hand in hand with our existing REST API 🙂

In this PR, I will attempt to address this issue: #124

I have tried to base my implementation off of the comments I could read in the issue and also in the PRs that were previously created for this. I also took inspiration from the Directive PR (#369) which helped me by showcasing recent good PR practices on the project.

I will leave several comments of my own where I am unsure about my approach, I would be grateful if you could let me know your views on them, and also for any time you would spend reviewing the PR.

I won't be very active during the upcoming week-end, but I'll definitely try to address any feedback as fast as I can during the next week.

Best!

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #521 into master will decrease coverage by 0.25%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
- Coverage   95.18%   94.93%   -0.26%     
==========================================
  Files          76       77       +1     
  Lines        1330     1361      +31     
  Branches      257      264       +7     
==========================================
+ Hits         1266     1292      +26     
- Misses         61       66       +5     
  Partials        3        3
Impacted Files Coverage Δ
src/schema/schema-generator.ts 96.74% <ø> (ø) ⬆️
src/metadata/utils.ts 90% <ø> (-4.12%) ⬇️
src/metadata/metadata-storage.ts 100% <100%> (ø) ⬆️
src/decorators/index.ts 100% <100%> (ø) ⬆️
src/decorators/Extensions.ts 88.88% <88.88%> (ø)
src/helpers/isThrowing.ts 20% <0%> (-60%) ⬇️

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 fe6b306...1fe39dc. Read the comment docs.

@hihuz hihuz marked this pull request as ready for review January 10, 2020 18:48
@MichalLytek MichalLytek self-requested a review January 13, 2020 20:17
@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request labels Jan 13, 2020
@MichalLytek MichalLytek added this to the 1.0.0 release milestone Jan 13, 2020
@MichalLytek
Copy link
Owner

@hihuz

image

image

@hihuz
Copy link
Contributor Author

hihuz commented Jan 14, 2020

@MichalLytek Hmm my mistake 🙁I did enable this option but it seems our organization rules override it, I have added an invitation on the repo for you, it should enable you to push changes if you accept it

If it does not, I will fork again on my personal account, sorry for the inconvenience.

@MichalLytek
Copy link
Owner

TypeError: Cannot read property 'log' of undefined",
  at LoggerMiddleware.use 

@hihuz I think you've forgot to setup container in buildSchema and haven't run the example 😄

@MichalLytek MichalLytek self-requested a review January 28, 2020 20:30
Copy link
Owner

@MichalLytek MichalLytek left a comment

Choose a reason for hiding this comment

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

Now everything looks great! 🎉

Thank you very much for the contribution ❤️

@hihuz
Copy link
Contributor Author

hihuz commented Jan 29, 2020

@MichalLytek Thank you very much for all the time you spent looking at the PR and for all the feedback, much appreciated 🙂

Apologies for the remaining issue with the examples 🤦‍♂ and thank you for fixing it 👍

@MichalLytek MichalLytek merged commit 1a72fcc into MichalLytek:master Jan 29, 2020
@MichalLytek MichalLytek deleted the feature/124-extensions-decorator branch January 29, 2020 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants