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

Improve crafting recipe documentation #12806

Merged
merged 3 commits into from Oct 1, 2022
Merged

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Sep 23, 2022

Fixes #12796.

This adds missing documentation for the crafting recipe definition in lua_api.txt. Currently, the "documentation" was mostly just examples, not a proper definition. So I have written the missing definition, and parameter specification, and stuff.

Just read the diff for details. :P

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Sep 28, 2022

Update! I have clarified some stuff about the crafting replacements and went into great detail, because those are pretty tricky.

Copy link
Contributor

@TurkeyMcMac TurkeyMcMac left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Some suggestions to shorten the text while keeping the important information.

doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Sep 30, 2022

I have accepted and applied most of these changes (new commit), except:

Crafting converts one or more inputs to one output ItemStack of arbitrary count
(including empty). The conversion reduces each input ItemStack by one (1).

This is wrong or misleading, the output can NOT be specified as an ItemStack because ItemStack is an object. I changed it to "item stack" that term doesn't imply an object.
The output is only empty for fuel recipes. Otherwise, output is required and Minetest even refuses to start if you have a non-fuel
recipe with empty output. I didn't explain that correctly tho.

Craft recipes are registered by minetest.register_craft. The accepted
formats are listed below..

The fact that it's a table still should be mentioned explicitly.

optional output

Is that actually possible? I haven't seen this being used anywhere.

Fuel recipes never have an output. I changed the text anyway because this was confusing.

What if the input stack has a count larger than 1? What's replaced?

I now explained this is more detail in a new bullet point.

[the first example]

I intentionally want to keep a long explanation for the first example. That's the point of having examples.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Sep 30, 2022

More reactions because I overlooked these:

  • output: Itemstring of the output ItemStack

Crafting behaviour (count >= 1) is already documented above.

Since this is the parameter list, I think it is OK to repeat it once. Also, ItemStack is misleading as it must not be an actual ItemStack object.

I suppose a commented code example would be more helpful than this text.

Replacing documentation by examples is exactly what documentation should NOT be doing. This was a direct reaction from a complaint in the issue this PR is reacting to. Also, examples are already provided. I consider this to be irreducible complexity.

@SmallJoker
Copy link
Member

This is wrong or misleading, the output can NOT be specified as an ItemStack because ItemStack is an object.

Interesting. ItemStacks are not accepted but any item string works, including tool wear - likely even with metadata. Weird but not related to PR :)

Replacing documentation by examples

I did not mean to replace the documentation, but to integrate it into a generic example to comprehend what's going on while seeing the table format. I wouldn't read the text and head on to the examples, but other people might act different. Fair enough.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Looks about right.

@ghost
Copy link

ghost commented Dec 4, 2022

Unfortunately 2x2 recipes are broken

@TurkeyMcMac
Copy link
Contributor

Could you open an issue?

@ghost
Copy link

ghost commented Dec 4, 2022

Could you open an issue?

Yep

appgurueu pushed a commit to appgurueu/minetest that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crafting recipe documentation is insufficient
3 participants