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

fix: default value set to true causes undefined method [] error #108

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

antoinesaliba
Copy link
Collaborator

There was a bug hidden by another partial bug. The hidden bug is here and it's just that it needs kwargs name and value. The bug that hid it was that the field.default check in the conditional if value.nil? && field.default will return false when the default value is false and it just so happened to be that our test struct defaults to false so line 39 was never tested.

include ActsAsComparable

const :tired, T::Boolean, default: false
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I debated just adding something to the job struct but it makes me nervous to keep adding things to that test struct because it's not easy to understand that every piece in there tests a number of things. I decided to just be more explicit and just create structs for specific testcases, this way we don't have one or a few central structs that hold everything together. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

Yea I think this is fine for testing a specific case.

@antoinesaliba antoinesaliba marked this pull request as ready for review July 3, 2024 20:53
Copy link
Owner

@maxveldink maxveldink left a comment

Choose a reason for hiding this comment

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

🦑

include ActsAsComparable

const :tired, T::Boolean, default: false
end
Copy link
Owner

Choose a reason for hiding this comment

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

Yea I think this is fine for testing a specific case.

@maxveldink maxveldink merged commit 6829bbf into main Jul 5, 2024
5 checks passed
@maxveldink maxveldink deleted the antoine.bux_fix_for_default branch July 5, 2024 02:37
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