Improve ability documentation #244

Zarel · 2018-10-15T00:33:46Z

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 · 2018-10-15T00:35:09Z

Codecov Report

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

@@            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 · 2018-10-16T00:52:31Z

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 · 2018-11-09T15:32:48Z

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

olistic

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

olistic · 2018-11-19T11:54:14Z

packages/warriorjs-abilities/src/attack.js

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


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.

olistic · 2018-11-19T12:05:19Z

packages/warriorjs-abilities/src/attack.js

  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).`,


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).`,

olistic · 2018-11-19T12:11:58Z

packages/warriorjs-abilities/src/directionOf.js

@@ -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) {


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) {

olistic · 2018-11-19T12:13:48Z

packages/warriorjs-abilities/src/feel.js

@@ -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).`,


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

olistic · 2018-11-19T12:14:11Z

packages/warriorjs-abilities/src/listen.js

@@ -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).',


Idem revert.

olistic · 2018-11-19T12:14:54Z

packages/warriorjs-abilities/src/look.js

@@ -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).`,


Idem revert (for the Space change only).

olistic · 2018-11-19T12:32:30Z

packages/warriorjs-abilities/src/rest.test.js

@@ -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.',


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.',

olistic · 2018-11-19T12:36:50Z

packages/warriorjs-cli/templates/readme/ability.ejs

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


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')

olistic · 2018-11-19T12:38:13Z

packages/warriorjs-core/src/getLevel.test.js

@@ -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).`,


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

Improve ability documentation

961c1d5

Merge branch 'master' into ability-doc

16369b7

olistic requested changes Nov 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ability documentation #244

Improve ability documentation #244

Zarel commented Oct 15, 2018

codecov bot commented Oct 15, 2018 •

edited

olistic commented Oct 16, 2018

Zarel commented Nov 9, 2018

olistic left a comment

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

olistic Nov 19, 2018

-    argumentsDescription: `direction = '${defaultDirection}'`,
+    params: [{
+      name: 'direction',
+      type: 'string',
+      description: 'The direction.',
+      defaultValue: `'${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).`,
	description: `Attacks a unit in the given direction (${defaultDirection} by default), dealing ${power} HP of damage (${backwardPower} HP if attacking backward).`,

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

	'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 -%>

Improve ability documentation #244

Are you sure you want to change the base?

Improve ability documentation #244

Conversation

Zarel commented Oct 15, 2018

codecov bot commented Oct 15, 2018 • edited

Codecov Report

olistic commented Oct 16, 2018

Zarel commented Nov 9, 2018

olistic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2018 •

edited