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

Type Validation for Var Operations and Enhanced Compatibility #1674

Merged
merged 37 commits into from
Sep 12, 2023

Conversation

ElijahAhianyo
Copy link
Contributor

@ElijahAhianyo ElijahAhianyo commented Aug 24, 2023

Description

This pull request addresses an issue where Var operations were previously allowed between any types, which led to unintended behaviors and potential edge cases. The purpose of this pull request is to align Var operations more closely with Python's type system and enforce type safety, especially considering that the codebase involves rendering operations in their JavaScript equivalents.

Changes Made

  • Added type checks and validation to all Var operations to ensure compatibility with Python's type system.
  • Refactored the code to use Python-specific type annotations and appropriate type hints.
  • Created type-specific checks for different operations, preventing unintended and potentially unsafe operations.
  • Introduced stricter type validation based on Python's type compatibility to prevent issues that could arise due to the permissive nature of JavaScript's type coercion.

Motivation

In the context of our codebase, which primarily involves writing code in Python, it's important to ensure that the operations performed between State Vars are compatible with Python's type rules and restrictions.
By limiting operations to be more Python-specific, we can avoid unexpected behaviors, potential type errors, and edge cases that might arise from JavaScript's permissive type system.
This change is intended to enhance the reliability and maintainability of the codebase while maintaining consistency with Python's type-safe practices.

Impact

This change enforces stricter type validation for Var operations, which might result in some previously valid operations being flagged as errors. However, these changes are in line with Python's type-safe philosophy and aim to prevent unexpected behaviors.
Developers who were relying on the previously permissive type handling might need to review their code to ensure compatibility with the new type restrictions.

@ElijahAhianyo ElijahAhianyo force-pushed the elijah/validate-var-operations branch from 89e7ee1 to 5404411 Compare August 29, 2023 14:44
@ElijahAhianyo ElijahAhianyo changed the title [WIP]Validate Var operations Validate Var operations Aug 29, 2023
@ElijahAhianyo ElijahAhianyo changed the title Validate Var operations Type Validation for Var Operations and Enhanced Compatibility Aug 29, 2023
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review August 29, 2023 21:35
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Nice, this approach is much cleaner!

@ElijahAhianyo ElijahAhianyo force-pushed the elijah/validate-var-operations branch from 2258f7a to 13536b7 Compare September 6, 2023 16:50
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple comments

@ElijahAhianyo ElijahAhianyo marked this pull request as draft September 8, 2023 13:28
@ElijahAhianyo ElijahAhianyo changed the title Type Validation for Var Operations and Enhanced Compatibility [WIP]Type Validation for Var Operations and Enhanced Compatibility Sep 8, 2023
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review September 12, 2023 15:16
@ElijahAhianyo ElijahAhianyo changed the title [WIP]Type Validation for Var Operations and Enhanced Compatibility Type Validation for Var Operations and Enhanced Compatibility Sep 12, 2023
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for adding the extensive tests also. Should we remove the bitwise operator before merging?

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

great work, looks good to me

@picklelo picklelo merged commit f2b0915 into main Sep 12, 2023
@picklelo picklelo deleted the elijah/validate-var-operations branch September 16, 2023 00:27
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