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 ability documentation #244

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Zarel
Copy link

@Zarel Zarel commented Oct 15, 2018

The warrior object isn't actually documented outside of the auto-generated README, so the auto-generated README could stand to be a bit more specific than it currently is.

Abilities now document arguments, and contain some other relevant information previously only found in tips.

@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

Merging #244 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   92.39%   92.68%   +0.28%     
==========================================
  Files          92       93       +1     
  Lines        1091     1093       +2     
  Branches      165      165              
==========================================
+ Hits         1008     1013       +5     
+ Misses         68       65       -3     
  Partials       15       15
Impacted Files Coverage Δ
packages/warriorjs-abilities/src/bind.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/rescue.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/shoot.js 100% <ø> (ø) ⬆️
...kages/warriorjs-abilities/src/directionOfStairs.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/listen.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/pivot.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/directionOf.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/detonate.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/feel.js 100% <ø> (ø) ⬆️
packages/warriorjs-abilities/src/rest.js 100% <ø> (ø) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97bac9f...16369b7. Read the comment docs.

@olistic
Copy link
Owner

olistic commented Oct 16, 2018

Hello @Zarel, just wanted to thank you for your contribution, and let you know that it didn't go by unnoticed. I'll be a bit busy for a couple more days, and then I'll review this.

@Zarel
Copy link
Author

Zarel commented Nov 9, 2018

Heya, just poking to make sure you didn't forget.

Copy link
Owner

@olistic olistic left a comment

Choose a reason for hiding this comment

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

Hey, sorry again for the delay. This is going to be a great addition, thanks! I made some comments/suggestions.

return unit => ({
action: true,
description: `Attacks a unit in the given direction (\`'${defaultDirection}'\` by default), dealing ${power} HP of damage.`,
argumentsDescription: `direction = '${defaultDirection}'`,
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rename argumentsDescription to params and include the name and type of the parameter, in addition to the description, in a more machine-friendly format. Then, when presenting the info to the user, we can choose what to display and in which format. Concretely, I suggest the following:

Suggested change
argumentsDescription: `direction = '${defaultDirection}'`,
params: [{
name: 'direction',
type: 'string',
description: 'The direction.',
defaultValue: `'${defaultDirection}'`,
}],

For the IntelliSense on warriorjs.com, I'm inferring those values from the description of the ability. This will allow me to remove that fragile code.

return unit => ({
action: true,
description: `Attacks a unit in the given direction (\`'${defaultDirection}'\` by default), dealing ${power} HP of damage.`,
argumentsDescription: `direction = '${defaultDirection}'`,
description: `Attacks a unit in the given direction (${defaultDirection} by default), dealing ${power} HP of damage in any direction except backward (reduced to ${backwardPower} if attacking backward).`,
Copy link
Owner

Choose a reason for hiding this comment

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

I like the addition you did! I find the wording unnecessarily long though. What about:

Suggested change
description: `Attacks a unit in the given direction (${defaultDirection} by default), dealing ${power} HP of damage in any direction except backward (reduced to ${backwardPower} if attacking backward).`,
description: `Attacks a unit in the given direction (${defaultDirection} by default), dealing ${power} HP of damage (${backwardPower} HP if attacking backward).`,

@@ -2,7 +2,7 @@ import { BACKWARD, FORWARD, LEFT, RIGHT } from '@warriorjs/geography';

function directionOf() {
return unit => ({
description: `Returns the direction (${FORWARD}, ${RIGHT}, ${BACKWARD} or ${LEFT}) to the given space.`,
description: `Returns the direction (\`'${FORWARD}'\`, \`'${RIGHT}'\`, \`'${BACKWARD}'\` or \`'${LEFT}'\`) to the given space.`,
perform(space) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is missing the params, and for Senses (which always return something), we could add a returns:

Suggested change
perform(space) {
params: [{
name: 'space',
type: 'Space',
description: 'The space.',
}],
returns: {
type: 'string'
description: 'The direction to the space.',
},
perform(space) {

@@ -4,7 +4,8 @@ const defaultDirection = FORWARD;

function feel() {
return unit => ({
description: `Returns the adjacent space in the given direction (\`'${defaultDirection}'\` by default).`,
argumentsDescription: `direction = '${defaultDirection}'`,
description: `Returns a \`Space\` object for the adjacent space in the given direction (${defaultDirection} by default).`,
Copy link
Owner

Choose a reason for hiding this comment

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

I'd revert this change once the returns is added.

@@ -3,7 +3,7 @@ import { FORWARD, getRelativeOffset } from '@warriorjs/geography';
function listen() {
return unit => ({
description:
'Returns an array of all spaces which have units in them (excluding yourself).',
'Returns an array of all `Space`s which have units in them (excluding yourself).',
Copy link
Owner

Choose a reason for hiding this comment

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

Idem revert.

@@ -4,7 +4,8 @@ const defaultDirection = FORWARD;

function look({ range }) {
return unit => ({
description: `Returns an array of up to ${range} spaces in the given direction (\`'${defaultDirection}'\` by default).`,
argumentsDescription: `direction = '${defaultDirection}'`,
description: `Returns an array of up to ${range} \`Space\`s in the given direction (${defaultDirection} by default).`,
Copy link
Owner

Choose a reason for hiding this comment

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

Idem revert (for the Space change only).

@@ -20,7 +20,7 @@ describe('rest', () => {

test('has a description', () => {
expect(rest.description).toBe(
'Gains 10% of max health back, but does nothing more.',
'Gains 10% of max health back (rounded half-up to the nearest integer), but does nothing more.',
Copy link
Owner

Choose a reason for hiding this comment

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

I think we don't need to clarify the rounding mode (it's what most people would expect).

Suggested change
'Gains 10% of max health back (rounded half-up to the nearest integer), but does nothing more.',
'Gains 10% of max health back (rounding to the nearest integer), but does nothing more.',

@@ -1 +1 @@
- `warrior.<%- ability.name %>()`: <%- ability.description -%>
- `warrior.<%- ability.name %>(<%- ability.argumentsDescription %>)`: <%- ability.description -%>
Copy link
Owner

Choose a reason for hiding this comment

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

We need to update this to use the new params (let's not show the type information here, just parameter name and default value if it exists). Examples:

warrior.directionOf(space)
warrior.walk(direction = 'forward')

@@ -26,14 +26,14 @@ const levelConfig = {
abilities: {
walk: () => ({
action: true,
description: `Moves one space in the given direction (\`'${FORWARD}'\` by default).`,
description: `Moves one space in the given direction (${FORWARD} by default).`,
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add the params and returns info here so we can test they are added to the level JSON.

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