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(utils): [closes #430] Make findInField return the correct plot #431

Merged
merged 4 commits into from
Aug 16, 2023

Conversation

jeremyckahn
Copy link
Owner

@jeremyckahn jeremyckahn commented Aug 16, 2023

What this PR does

For #430, this PR fixes an implementation bug that manifested in Scarecrows not working if they weren't the first plot in a Field row.

How this change can be validated

Place a Scarecrow somewhere in the middle of the Field and ensure that crows do not attack when ending the day.

Additional information

It seems that this bug may have been introduced here: https://github.com/jeremyckahn/farmhand/pull/427/files#diff-ebd8554ec43b07366bd40d16b09d1d063d9e8d5921fa30de4eb80bffdf82668dR718-R727

In any case, this issue wasn't noticed until #427 was shipped.

@jeremyckahn jeremyckahn added the bug Something isn't working label Aug 16, 2023
@vercel
Copy link

vercel bot commented Aug 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
farmhand ✅ Ready (Inspect) Visit Preview Aug 16, 2023 3:01pm

* @returns {?farmhand.plotContent}
*/
(field, condition) => {
const [plot = null] =
Copy link
Owner Author

Choose a reason for hiding this comment

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

The root cause of the bug is that this is always returning the first plot in the row.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohhh, wow good find. i didn't even notice this syntax when i was looking yesterday

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah... that's what I get for being clever. 🤦

The new version is much less clever and more straightforward, I think!

* @returns {?farmhand.plotContent}
*/
(field, condition) => {
const [plot = null] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

ohhh, wow good find. i didn't even notice this syntax when i was looking yesterday

@jeremyckahn jeremyckahn merged commit f0faa0f into develop Aug 16, 2023
6 checks passed
@jeremyckahn jeremyckahn deleted the fix/430__find-in-field branch August 16, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants