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

feat: adds tests and docs for @deepkit/type #602

Merged
merged 31 commits into from
Sep 7, 2021
Merged

Conversation

smolinari
Copy link
Contributor

@smolinari smolinari commented Sep 2, 2021

This PR adds tests and some documentation for using @deepkit/type with Typegoose for serialization and deserialization.

Related Issues

Scott

@smolinari smolinari changed the title feat: adds tests and docs for @typekit/type feat: adds tests and docs for @deepkit/type Sep 2, 2021
@hasezoey hasezoey linked an issue Sep 2, 2021 that may be closed by this pull request
@hasezoey hasezoey added docs This issue is about Documentation tests This Issue is about tests | This PR is modifying tests labels Sep 2, 2021
Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Thanks for the documentation about @deepkit/type
there are some things needing to be fixed in this pr:

  • please resolve the merge conflicts (currently package.json)
  • please remove package-lock.json and use yarn
  • maybe re-commit with a signed / registered commit (like 9f2160a)

@smolinari
Copy link
Contributor Author

@hasezoey - I have to ask the stupid question, what is a signed / registered commit?

Scott

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

I have to ask the stupid question, what is a signed / registered commit?

sorry if i have put it weirdly, i meant for you to either:

  • sign the commit that is currently there
  • or "registering" (changing) your commit (to be) like previously in 9f2160a

and i ask this because it seems your current commit in this PR is not "linked" to your account (also, to have it accounted into one contributor in the git log)

Note: this is just a suggestion, because i have seen it done previously in the mentioned commit

@smolinari
Copy link
Contributor Author

@hasezoey - I got the GPG key going. Funny thing is, I'm certain I didn't have that when I did the other PR.

I'm also not sure why there are these conflicts. What am I doing wrong? 🤔

Scott

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

I got the GPG key going. Funny thing is

the second commit in this PR has now a signature, but it does not seem like it is added to your account (so it is unverified)

I'm certain I didn't have that when I did the other PR.

maybe you did the commit with the github web interface?


and as for the current commits not being accounted / linking to your account is probably because no email that is also used for your github is provided in the commits

Edit:
Email used last time: smolinari[@]users.noreply.github.com
Email used now: scott.molinari[@]m8a.io
and it seems like the "new" Email is not added as a official email in your github account
(Note: escaped the emails with [])

I'm also not sure why there are these conflicts. What am I doing wrong? thinking

your current beta (this branch) seems to be based on a 8.0.0-beta.x commit, try updating your beta branch to this repositories (your PR base is 1577b36)

@smolinari
Copy link
Contributor Author

smolinari commented Sep 2, 2021

I've gotten the commit to be verified.

What would you like me to do now though? 😁

Scott

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Please rebase onto the latest upstream beta branch (yours is way behind)

@smolinari
Copy link
Contributor Author

I have no clue, if I did it right or not. Very new to rebasing. 🤷 If not, I'll close this PR and start a new one on a fresh fork.

Scott

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

yes it seems like you rebased onto the upstream beta, but somehow fc63803 came from that, which basically reverts most of the stuff back again

do you want me to rebase this branch?

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

Rebased onto latest upstream beta, also dropped the problematic extra commits and squashed the package-lock.json -> yarn into the one before

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #602 (1c24c7c) into beta (8e7597d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             beta     #602   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files          17       17           
  Lines         839      839           
  Branches      222      222           
=======================================
  Hits          788      788           
  Misses         48       48           
  Partials        3        3           

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 8e7597d...1c24c7c. Read the comment docs.

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

also changed commiter info if the first commit (to be the same as the later ones)

PS: also thanks to the rebase re-commiting the commits they are now signed with my key (you are the author, i am the commiter - see this stackoverflow question)

@hasezoey
Copy link
Member

hasezoey commented Sep 2, 2021

if you have problems updating your local branch, run (while being in the branch of this pr) git pull -f

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Some things i noticed now that everything is in order and "working"

test/tests/deepkitType.test.ts Outdated Show resolved Hide resolved
test/tests/deepkitType.test.ts Show resolved Hide resolved
test/tests/deepkitType.test.ts Show resolved Hide resolved
docs/guides/advanced/deepkit-type.md Show resolved Hide resolved
docs/guides/advanced/deepkit-type.md Show resolved Hide resolved
docs/guides/advanced/deepkit-type.md Outdated Show resolved Hide resolved
docs/guides/advanced/deepkit-type.md Show resolved Hide resolved
docs/guides/advanced/deepkit-type.md Outdated Show resolved Hide resolved
@smolinari
Copy link
Contributor Author

smolinari commented Sep 3, 2021

to summarize, the only changes left to do are:

  • add the "groups are slow" statement to the documentation as a reference (fa5fa8e)
  • change the test normal query & documentation to also use plainToClass

@hasezoey - I'd like to do this, but like I mentioned above, my repo is borked. What would you suggest I should so as not to mess things up even more?

Scott

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

everything now looks fine, @smolinari are you fine with the changes and how the documentation looks?

(you can run docusaurus manually, see website/README)

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

again, everything looks still fine, @smolinari are you fine with the changes and how the documentation looks?

(you can run docusaurus manually, see website/README)

@hasezoey hasezoey merged commit 38a9d02 into typegoose:beta Sep 7, 2021
@github-actions
Copy link

github-actions bot commented Sep 7, 2021

🎉 This PR is included in version 9.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This issue is about Documentation released on @beta released tests This Issue is about tests | This PR is modifying tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test cases and documentation to use @deepkit/type
2 participants