-
Notifications
You must be signed in to change notification settings - Fork 488
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,15 +3,17 @@ import { BACKWARD, FORWARD } from '@warriorjs/geography'; | |||||
const defaultDirection = FORWARD; | ||||||
|
||||||
function attack({ power }) { | ||||||
const backwardPower = Math.ceil(power / 2.0); | ||||||
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).`, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
perform(direction = defaultDirection) { | ||||||
const receiver = unit.getSpaceAt(direction).getUnit(); | ||||||
if (receiver) { | ||||||
unit.log(`attacks ${direction} and hits ${receiver}`); | ||||||
const attackingBackward = direction === BACKWARD; | ||||||
const amount = attackingBackward ? Math.ceil(power / 2.0) : power; | ||||||
const amount = attackingBackward ? backwardPower : power; | ||||||
unit.damage(receiver, amount); | ||||||
} else { | ||||||
unit.log(`attacks ${direction} and hits nothing`); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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) { | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is missing the
Suggested change
|
||||||||||||||||||||||||
return unit.getDirectionOf(space); | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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).`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd revert this change once the |
||
perform(direction = defaultDirection) { | ||
return unit.getSensedSpaceAt(direction); | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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).', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idem revert. |
||
perform() { | ||
return unit | ||
.getOtherUnits() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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).`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idem revert (for the |
||
perform(direction = defaultDirection) { | ||
const offsets = Array.from(new Array(range), (_, index) => index + 1); | ||
const spaces = offsets.map(offset => | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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.', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
); | ||||||
}); | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
- `warrior.<%- ability.name %>()`: <%- ability.description -%> | ||
- `warrior.<%- ability.name %>(<%- ability.argumentsDescription %>)`: <%- ability.description -%> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to update this to use the new
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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).`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add the |
||
}), | ||
attack: () => ({ | ||
action: true, | ||
description: `Attacks a unit in the given direction (\`'${FORWARD}'\` by default), dealing 5 HP of damage.`, | ||
description: `Attacks a unit in the given direction (${FORWARD} by default), dealing 5 HP of damage.`, | ||
}), | ||
feel: () => ({ | ||
description: `Returns the adjacent space in the given direction (\`'${FORWARD}'\` by default).`, | ||
description: `Returns the adjacent space in the given direction (${FORWARD} by default).`, | ||
}), | ||
}, | ||
position: { | ||
|
@@ -51,10 +51,10 @@ const levelConfig = { | |
abilities: { | ||
attack: () => ({ | ||
action: true, | ||
description: `Attacks a unit in the given direction (\`'${FORWARD}'\` by default), dealing 3 HP of damage.`, | ||
description: `Attacks a unit in the given direction (${FORWARD} by default), dealing 3 HP of damage.`, | ||
}), | ||
feel: () => ({ | ||
description: `Returns the adjacent space in the given direction (\`'${FORWARD}'\` by default).`, | ||
description: `Returns the adjacent space in the given direction (${FORWARD} by default).`, | ||
}), | ||
}, | ||
playTurn(sludge) { | ||
|
@@ -145,19 +145,19 @@ test('returns level', () => { | |
{ | ||
name: 'attack', | ||
description: | ||
"Attacks a unit in the given direction (`'forward'` by default), dealing 5 HP of damage.", | ||
'Attacks a unit in the given direction (forward by default), dealing 5 HP of damage.', | ||
}, | ||
{ | ||
name: 'walk', | ||
description: | ||
"Moves one space in the given direction (`'forward'` by default).", | ||
'Moves one space in the given direction (forward by default).', | ||
}, | ||
], | ||
senses: [ | ||
{ | ||
name: 'feel', | ||
description: | ||
"Returns the adjacent space in the given direction (`'forward'` by default).", | ||
'Returns the adjacent space in the given direction (forward by default).', | ||
}, | ||
], | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename
argumentsDescription
toparams
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: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.