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

Replacing any keyword instances with relevant data types #1040

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

Conversation

Asymtode712
Copy link

@Asymtode712 Asymtode712 commented Oct 18, 2024

What kind of change does this PR introduce?

  • Refactoring

Issue Number:

Screenshots/videos:

If relevant, did you update the documentation?

Summary

This PR addresses the issue regarding use of the any datatype across several places in the codebase. The any type alters the strictness of TypeScript, which is not a good practice. The goal of this PR is to replace instances of any with correct data type definitions to enhance type safety. In some cases, custom data types have been created to ensure clarity and maintainability of the code. This change aims to improve code quality and maintain TypeScript's strict type-checking features.

Does this PR introduce a breaking change?

No, this PR does not introduce any breaking changes. All existing functionalities remain intact; however, the internal implementation has been improved with better type definitions.

Note to Maintainers:
Most of the components and pages are now free from the recurring use of any keyword except one component i.e., JsonEditor. While I aimed to replace all instances of the any keyword in the JsonEditor component, some of the types were difficult to define precisely due to the dynamic nature of JSON data. Since JSON can vary widely in structure and content, there were certain cases where assigning a specific type could lead to potential type mismatches or errors in runtime

@Asymtode712 Asymtode712 requested a review from a team as a code owner October 18, 2024 18:48
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to the JSON Schema Community. Thanks a lot for creating your first pull request!! 🎉🎉 We are so excited you are here! We hope this is only the first of many! For more details check out README.md file.

Copy link

github-actions bot commented Oct 18, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
website ✅ Ready (View Log) Visit Preview 42c7872

@Asymtode712 Asymtode712 changed the title Typefix Replacing any keyword instances with relevant data types Oct 18, 2024
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7ecd915) to head (42c7872).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1040   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          373       373           
  Branches        94        94           
=========================================
  Hits           373       373           

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

@Asymtode712
Copy link
Author

@DhairyaMajmudar @benjagm Please review this PR

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

@Asymtode712 Thank you for the great work done in the PR pls. address the suggestions and some questions also move all the custom data type blocks at the starting of the file after import statements.

components/Code.tsx Outdated Show resolved Hide resolved
components/Headlines.tsx Outdated Show resolved Hide resolved
components/Headlines.tsx Outdated Show resolved Hide resolved
components/Layout.tsx Outdated Show resolved Hide resolved
components/Layout.tsx Outdated Show resolved Hide resolved
pages/specification/release-notes/index.page.tsx Outdated Show resolved Hide resolved
pages/tools/components/ToolingTable.tsx Outdated Show resolved Hide resolved
pages/tools/components/ToolingTable.tsx Outdated Show resolved Hide resolved
pages/understanding-json-schema/[slug].page.tsx Outdated Show resolved Hide resolved
pages/understanding-json-schema/reference/[slug].page.tsx Outdated Show resolved Hide resolved
@Asymtode712
Copy link
Author

@DhairyaMajmudar I have carried out all the changes which were requested by you....please have a look at the PR now

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

Amazing PR 🚀
Left few more comments then we are good to go

components/Headlines.tsx Outdated Show resolved Hide resolved
components/Layout.tsx Outdated Show resolved Hide resolved
pages/obsolete-implementations/index.page.tsx Outdated Show resolved Hide resolved
@DhairyaMajmudar
Copy link
Member

@benjagm I've seen at many occurrences we are using almost the same type definitions, what are your views on creating a global types files and moving such common type definitions into it .... furthermore simply importing them on required locations?

@Asymtode712
Copy link
Author

@DhairyaMajmudar I am done with the remaining changes as requested by you.....Kindly review them once

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@DhairyaMajmudar DhairyaMajmudar added the Hacktoberfest-accepted Pull requests accepted for Hacktoberfest'24 label Oct 19, 2024
@Asymtode712
Copy link
Author

@benjagm Please review this PR and merge it if it is good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest-accepted Pull requests accepted for Hacktoberfest'24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants