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

[typescript-migration] fix types & imports #4747

Merged
merged 1 commit into from
May 3, 2024

Conversation

yuki0410-dev
Copy link
Contributor

name: fix types & imports

Description

Linked issue: #4700

Changes fix types & imports

Contribution checklist

  • I have followed the contributing guidelines.
  • I have added sufficient test coverage for my changes.
  • I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

⚠️ This pull request was not sent to the PullRequest network because the pull request is a draft.

@yuki0410-dev
Copy link
Contributor Author

yuki0410-dev commented Apr 30, 2024

#4740 will be merged and then Draft will be released.

@yuki0410-dev
Copy link
Contributor Author

If #4743 is merged first, an additional fix will be made.

@yuki0410-dev yuki0410-dev force-pushed the feat/4700_types branch 2 times, most recently from 4efe60e to 286ff81 Compare April 30, 2024 02:17
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.35%. Comparing base (30d9edb) to head (4b38122).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4747   +/-   ##
=======================================
  Coverage   96.35%   96.35%           
=======================================
  Files           8        8           
  Lines         959      959           
  Branches      435      442    +7     
=======================================
  Hits          924      924           
  Misses         35       35           

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

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:
  • Comments posted to any areas of potential concern or improvement.
  • Detailed feedback or actions needed to resolve issues that are found.
  • Turnaround times vary, but we aim to be swift.

@yuki0410-dev you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 278
- 237

89% TSX
10% TypeScript
1% JavaScript (tests)

Type of change

Fix - These changes are likely to be fixing a bug or issue.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Just one nit about possible extraneous annotations. Other than that, the PR looks good! All annotations seem correct, so my nit is certainly not blocking.

Image of Jacob Jacob


Reviewed with ❤️ by PullRequest

const maxYear = this.props.maxDate ? getYear(this.props.maxDate) : 2100;

const options = [];
const minYear: number = this.props.minDate
Copy link

Choose a reason for hiding this comment

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

Is this annotation necessary? It seems like it should be trivially inferred based on the arms, and it shouldn't be confusing to humans due to the presence of a numeric literal.

🔹 Simplify Code (Nice to have)

Image of Jacob Jacob

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review.
I thought it would be better to state explicitly here that I want to get a NUMBER (without resorting to inference) from the results of multiple conditional branches.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

I looked over this pull request thoroughly, but it was difficult to find any problems with it. Nice work! I appreciated the attention to detail and simplification of imports and code throughout the changes.

Image of Jonah S Jonah S


Reviewed with ❤️ by PullRequest

@@ -55,11 +55,16 @@ import { isWithinInterval } from "date-fns/isWithinInterval";
import { toDate } from "date-fns/toDate";
import { parse } from "date-fns/parse";
import { parseISO } from "date-fns/parseISO";
import { type Locale, addSeconds, Day } from "date-fns";
import { type Locale as DateFnsLocale, addSeconds, Day } from "date-fns";
Copy link

Choose a reason for hiding this comment

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

Thank you for disambiguating the date-fns Locale and the date_utils Locale!

◽ Compliment

Image of Jonah S Jonah S

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4747 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Image of Jacob Jacob

@martijnrusschen
Copy link
Member

Looks like there're some conflicts on this one.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4747 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Image of Jonah S Jonah S

@yuki0410-dev
Copy link
Contributor Author

@martijnrusschen
Rebased and fixed.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #4747 up until the latest commit (d897045). No further issues were found.

Reviewed by:

Image of Jonah S Jonah S

@yuki0410-dev
Copy link
Contributor Author

error - 2024-05-03 15:39:57,426 -- Commit creating failed: {"detail":"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 40 seconds."}
Error: Codecov: Failed to properly create commit: The process '/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov' failed with exit code 1

@yuki0410-dev
Copy link
Contributor Author

@martijnrusschen
Rebased and passed all ci.

@martijnrusschen martijnrusschen merged commit 27829c7 into Hacker0x01:main May 3, 2024
6 checks passed
@yuki0410-dev yuki0410-dev deleted the feat/4700_types branch May 3, 2024 15:54
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