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

Add truncate functionality #162

Open
wants to merge 6 commits into
base: master
from
Open

Conversation

@patrickdronk
Copy link

@patrickdronk patrickdronk commented Nov 8, 2020

I've added the truncate functionality as described here
Things that should be done after merge, which I'll do as well:

  • Update docs
  • Checks? I've seen this directory. But not sure what the purpose of it is. Is there something I can read to get some context?
@martijndeh martijndeh self-requested a review Nov 8, 2020
Copy link
Contributor

@martijndeh martijndeh left a comment

Great, you pretty much nailed it! There are some minor clean ups possible but that's fine. Are you running prettier? I should probably add a npm script for this.

As for the checks yes that's a good point which I should explain a bit better. The checks are basically type-checking unit tests and makes sure we don't create any regressions with the types. With all the advanced typings going on it's very easy to make a mistake and get an any somewhere. Regular runtime unit tests don't really cover this so we use dts-jest. https://www.npmjs.com/package/dts-jest

It would be nice to get at least one check in to see the return type of the truncate query. I'm assuming this should be a number. The delete check is actually very similar (and simple), so you could try and copy that one. What do you think?

It looks like the tests etc aren't running on this branch yet so I have some work to do. I'll get this sorted in the next couple of days.

Good stuff!

README.md Outdated Show resolved Hide resolved
src/truncate.ts Outdated Show resolved Hide resolved
@martijndeh martijndeh linked an issue that may be closed by this pull request Nov 8, 2020
@patrickdronk
Copy link
Author

@patrickdronk patrickdronk commented Nov 16, 2020

Hi @martijndeh
Thanks for your feedback, I've added the prettier package now (locally). Styling should be more in line with the project now.
I've read up on the type testing. I've basically used the delete, and changed it ^^. I hope it is what you expect from it.
Let me know what you think of the last changes 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.