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

Added date validation #93

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Added date validation #93

wants to merge 3 commits into from

Conversation

FreezyEx
Copy link

Added Date validation to the Form class.
Valid syntax defined here

@KnorpelSenf
Copy link
Member

KnorpelSenf commented Dec 11, 2023

Cool idea, it looks good on first sight, but I'll do a proper review tonight. Could you fix CI in the meantime?

src/form.ts Outdated
const ctx = await this.conversation.wait();
const text = ctx.msg?.text ?? ctx.msg?.caption ?? "";
let date = new Date(text);
if(date === "Invalid Date") {
Copy link
Member

Choose a reason for hiding this comment

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

This comparison looks sketchy, though, are you sure that dates work like that?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed converting date to a string

@KnorpelSenf
Copy link
Member

Please revert that fix. You committed a pnpm lock file and reformatted the entire project.

If you do not have Deno installed or are unsure how to fix up CI, just drop me a comment and I can fix it up :)

@FreezyEx
Copy link
Author

Please revert that fix. You committed a pnpm lock file and reformatted the entire project.

If you do not have Deno installed or are unsure how to fix up CI, just drop me a comment and I can fix it up :)

Installed Deno and reverted. I cannot get format-and-lint to work without modifying the whole file with deno fmt

@KnorpelSenf
Copy link
Member

KnorpelSenf commented Dec 12, 2023

I can fix it up later today

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (3124f1f) 95.83% compared to head (8a8fe76) 94.60%.

Files Patch % Lines
src/form.ts 8.33% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
- Coverage   95.83%   94.60%   -1.23%     
==========================================
  Files           4        4              
  Lines         840      853      +13     
  Branches      124      124              
==========================================
+ Hits          805      807       +2     
- Misses         33       44      +11     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KnorpelSenf
Copy link
Member

Oh nice, you got it already

@FreezyEx
Copy link
Author

Oh nice, you got it already

Do I need to do anything else?

@roziscoding
Copy link
Contributor

Oh nice, you got it already

Do I need to do anything else?

it seems the job is still failing. @KnorpelSenf do you wanna take a look at it?

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.

3 participants