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

feat: Grapes #393

Merged
merged 29 commits into from
Feb 20, 2023
Merged

feat: Grapes #393

merged 29 commits into from
Feb 20, 2023

Conversation

jeremyckahn
Copy link
Owner

@jeremyckahn jeremyckahn commented Feb 2, 2023

What this PR does

This PR adds Grape crops. There are ten types of Grapes, and they grow randomly from the one Grape Seed item. In order to support this, the crop system was extended to generically support varieties.

How this change can be validated

It's easiest to set yourself up with a high level, lots of money, and no inventory limit to test this:

farmhand.setState({
  itemsSold: { carrot: 200000 },
  money: 10000000,
  inventoryLimit: -1
}) 

From there buy Grape Seeds from the Shop and go through a few harvest cycles. It would be good to keep an eye on the price events to see that the various Grape types show up there appropriately (though I've tested that and it seemed stable to me).

Questions or concerns about this change

Grapes are unlocked at level 42. Would it be worth introducing this more exotic item earlier in the game by swapping it for another, lower-tiered item?

Additional information

Gameplay screenshots

Planted grape seeds

Growing grapes

Mature grapes

Grapes in the inventory

The addition of support for crop varieties touched a lot of code, so this is a rather large PR. Subsequent PRs adding new crops with varieties should not be nearly so large.

In particular, this PR updates a lot of tests. Broadly speaking, Farmhand's unit tests are overly dependent on mock item data. We probably shouldn't be doing this at all going forward. It creates needless abstraction, and much of the game's item data is stable enough to build tests on top of. It seems worth migrating tests to use real item data as we update them, and that long-term effort is started here.

This PR also introduces the use JSDoc import types. While won't be migrating to TypeScript any time in the foreseeable future, we can still enjoy the editor conveniences of rich type system. This won't prevent us from misusing types, as that's a problem that only the complete adoption of a type system can provide. JSDoc import types are just an iterative step towards a better codebase, and it will also be an ongoing improvement effort to enhance files with as we touch them. To be clear: This isn't a new development requirement, just a handy thing we can take advantage of if we feel like it as we go.

Here's what we get in an editor that's configured to take advantage of TypeScript (even though these are JavaScript files)
Screencast.from.02-02-2023.09.32.12.AM.webm

@vercel
Copy link

vercel bot commented Feb 2, 2023

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

Name Status Preview Comments Updated
farmhand ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 1:27AM (UTC)

@@ -0,0 +1,28 @@
/**
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is taken from here.

@@ -47,7 +47,7 @@ describe('private helpers', () => {

describe('getPlantableCropInventory', () => {
test('selects plantable crop items from inventory', () => {
const inventory = [{ id: 'sample-crop-seeds-1' }, { id: 'sample-item-1' }]
const inventory = [{ id: 'sample-crop-1-seed' }, { id: 'sample-item-1' }]
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not all of the tests touched by this PR were updated to use actual item data in order to reign in the scope. Part of setting up the tests to validate crop variety logic involved fixing some mock data. This seems to support the case to move away from it completely.

Comment on lines +1 to +2
/** @typedef {import("../../index").farmhand.item} farmhand.item */
/** @typedef {import("../../index").farmhand.plotContent} farmhand.plotContent */
Copy link
Owner Author

Choose a reason for hiding this comment

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

These are the JSDoc import types I was referencing in the PR description. I plan to add these as I update the relevant parts of the codebase. We'll probably never have complete typing, but more is better than less. 🙂

Copy link
Collaborator

@lstebner lstebner left a comment

Choose a reason for hiding this comment

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

In particular, this PR updates a lot of tests. Broadly speaking, Farmhand's unit tests are overly dependent on mock item data. We probably shouldn't be doing this at all going forward. It creates needless abstraction, and much of the game's item data is stable enough to build tests on top of. It seems worth migrating tests to use real item data as we update them, and that long-term effort is started here.

strongly agree!

honestly everything here looks reasonable and solid. my only concern is that this is too many varieties, but no issues with the code and i think it's ultimately up to you on the number of varieties. i hope we can utilize this with more crops in the future! or maybe even some of the pre-existing ones like corn, and carrots.

as far as if we should add this earlier in the game level wise i don't necessarily think we need to. i still think we should give players the ability to level up quicker so more of them get to see more content, but i can't think of any specific advantage of having grapes earlier than something else. at least, not until we have kegs in the workshop.

src/data/crops/grape.js Outdated Show resolved Hide resolved
@jeremyckahn
Copy link
Owner Author

my only concern is that this is too many varieties, but no issues with the code and i think it's ultimately up to you on the number of varieties.

Agreed. I've cut out half of them for now and this is what remains:

Screenshot

Final grape variety list

i still think we should give players the ability to level up quicker so more of them get to see more content, but i can't think of any specific advantage of having grapes earlier than something else. at least, not until we have kegs in the workshop.

💯! That all makes sense to me.

@jeremyckahn jeremyckahn merged commit 3dd5cff into develop Feb 20, 2023
@jeremyckahn jeremyckahn deleted the feature/375__grapes branch February 20, 2023 16:39
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.

None yet

2 participants