-
Notifications
You must be signed in to change notification settings - Fork 2
Value Equals fix; switch style fix #13
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, 2 comments
ValueSystem/CollectionValue.cs
Outdated
| } = null!; | ||
|
|
||
|
|
||
| public override bool EqualCondition(Value other) => other is CollectionValue otherP && CastedValues.SequenceEqual(otherP.CastedValues) && Type == otherP.Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: SequenceEqual uses default equality comparison which may not invoke the custom EqualCondition methods on Value elements. Consider using a custom comparer or manually iterating through elements to call EqualCondition. Should the collection equality check use the custom EqualCondition method for comparing individual Value elements instead of relying on default equality?
Prompt To Fix With AI
This is a comment left during a code review.
Path: ValueSystem/CollectionValue.cs
Line: 53:53
Comment:
**logic:** SequenceEqual uses default equality comparison which may not invoke the custom EqualCondition methods on Value elements. Consider using a custom comparer or manually iterating through elements to call EqualCondition. Should the collection equality check use the custom EqualCondition method for comparing individual Value elements instead of relying on default equality?
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tosoks67 weź rzuć okiem na to, chyba dobry catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dodany fix do pull requesta
ValueSystem/ReferenceValue.cs
Outdated
| public bool IsValid => value is not null; | ||
| public object Value => value ?? throw new ScriptRuntimeError("Value of reference is invalid."); | ||
|
|
||
| public override bool EqualCondition(Value other) => other is ReferenceValue otherP && Value.Equals(otherP.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Calling Value property on invalid references will throw ScriptRuntimeError. Consider checking IsValid on both references before accessing Value properties to avoid exceptions during equality comparisons. Should equality comparison between invalid references return false, or should it throw an exception as currently implemented?
Prompt To Fix With AI
This is a comment left during a code review.
Path: ValueSystem/ReferenceValue.cs
Line: 11:11
Comment:
**logic:** Calling `Value` property on invalid references will throw `ScriptRuntimeError`. Consider checking `IsValid` on both references before accessing `Value` properties to avoid exceptions during equality comparisons. Should equality comparison between invalid references return false, or should it throw an exception as currently implemented?
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, no comments
Elektryk-Andrzej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Added an abstract bool method
EqualConditiontoValue(takesValueas the only parameter; It's basically the condition that's checked in theEqualsoverride and the==and!=)made the switches look good (perfectionist ahh)