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

Add comments, fix code on HasIterator and TypeClassification #26

Merged
merged 8 commits into from
Oct 20, 2024

Conversation

adamusgr
Copy link
Contributor

1.Added comments for each method, making the code more understandable and maintainable.

2.Corrected return types and method parameters, adding more checks and accuracy in data type handling.

3.Ensured that the toEnum() method properly checks the values to return the correct classification types (IncomeClassificationType or ExpenseClassificationType).

1.Added comments for each method, making the code more understandable and maintainable.

2.Corrected return types and method parameters, adding more checks and accuracy in data type handling.

3.Ensured that the toEnum() method properly checks the values to return the correct classification types (IncomeClassificationType or ExpenseClassificationType).
@adamusgr
Copy link
Contributor Author

I cant understand what is wrong with the
$this->assertTrue($classifications->get(IncomeClassificationCategory::CATEGORY_1_95)->contains(null));

@firebed
Copy link
Owner

firebed commented Oct 16, 2024

null is a valid classification type for classification category categor1_95. In your PR you replaced

return empty($this->toArray()) || in_array($value, $this->toArray(), true);

with this

return empty($this->classifications);

This bypasses the check for null classification type. Also, I don't understand the reason behind completely ditching the TypeClassificationCollection::toArray() method. I believe I used the toArray() method internelly in the class for cosistency.

@adamusgr
Copy link
Contributor Author

adamusgr commented Oct 16, 2024

Oh yes!! Sorry, but I didn’t quite understand the class correctly. Your code is soooo large, and honestly, you've done a really great job.

I restored the functions to their original state. I left the comments.

@firebed
Copy link
Owner

firebed commented Oct 16, 2024

Frankly, I wouldn't touch a code if I don't fully understand it 😃

On a second thought, it looks better accessing the classifications property directly via $this->classifications within the class itself, so what you did was indeed on point. If ever we need to add a specific logic (like formatting or filtering), it makes more sense to put this logic inside the toArray() method without worrying if we break the internal use case.

I know that you reverted the change, but is this something you would like to re-implement?
I noticed some PSR-12 issues. Before merging pleaase see that there are no extra spaces and indentation is correct (lines 43 and 176).

src/Traits/HasIterator.php Outdated Show resolved Hide resolved
src/Traits/HasIterator.php Outdated Show resolved Hide resolved
@adamusgr
Copy link
Contributor Author

**

Frankly, I wouldn't touch a code if I don't fully understand it 😃

On a second thought, it looks better accessing the classifications property directly via $this->classifications within the class itself, so what you did was indeed on point. If ever we need to add a specific logic (like formatting or filtering), it makes more sense to put this logic inside the toArray() method without worrying if we break the internal use case.

I know that you reverted the change, but is this something you would like to re-implement? I noticed some PSR-12 issues. Before merging pleaase see that there are no extra spaces and indentation is correct (lines 43 and 176).

For now, I'll leave it as it is until I fully understand your logic. If you still think we should make the change, that's okay.

@firebed
Copy link
Owner

firebed commented Oct 16, 2024

I could proceed to make the changes, if you'd prefer.

@adamusgr
Copy link
Contributor Author

I could proceed to make the changes, if you'd prefer.

Yes go on!!!

@firebed firebed merged commit a8fe1f7 into firebed:5.x Oct 20, 2024
1 check passed
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