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

Doc fixes #1197

Merged
merged 5 commits into from
Feb 19, 2017
Merged

Doc fixes #1197

merged 5 commits into from
Feb 19, 2017

Conversation

jamesandrewclarke
Copy link

@jamesandrewclarke jamesandrewclarke commented Feb 18, 2017

Improve consistency in examples

@iCrawl
Copy link
Member

iCrawl commented Feb 19, 2017

If you could elaborate how this makes it better?
Also switching from reject to console.error doesn't seem better to me in any way.

@@ -573,7 +573,9 @@ class Guild {
* be resolved, the user ID will be the result.
* @example
* // ban a user
* guild.ban('123123123123');
* guild.ban(user)
Copy link
Author

Choose a reason for hiding this comment

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

'user' instead of '123123123'

Copy link
Member

Choose a reason for hiding this comment

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

But you don't explain what "user" is at all.
At the same time you add a promise where you pass the user if it resolves and console log it, but guild.ban can be used without the user being on the server initially, so you would have no user to resolve.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, expansion on what user is could be beneficial, but this is still an improvement on 123123123.
As for bans being invoked before the user is in the guild, that's news to me. Do you know of a way to resolve a user outside of any guilds the bot is in?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -573,7 +573,9 @@ class Guild {
* be resolved, the user ID will be the result.
* @example
* // ban a user
* guild.ban('123123123123');
* guild.ban(user)
* .then(user => console.log(`Banned ${user} from ${guild.name}`))
Copy link
Author

Choose a reason for hiding this comment

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

.then() statement is also in line with other examples

* .then(user => console.log(`Unbanned ${user.username} from ${guild.name}`))
* .catch(reject);
* .catch(console.error);
Copy link
Author

Choose a reason for hiding this comment

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

Use of 'console.error' is more in line with the rest of the examples

@aemino
Copy link
Contributor

aemino commented Feb 19, 2017

Aside from what Crawl pointed out I feel like this PR is too specific. If you'd like to help us out with the documentation, that's great, but your PRs should cover a more diverse set of code. For example, I'm sure some of the consistency changes you made in this PR could've been applied in other areas too...

@Gawdl3y
Copy link
Member

Gawdl3y commented Feb 19, 2017

The change from reject to console.error is 👍 for sure. The switch from an ID to a nondescript variable, however, isn't. You're right in that the current "ID" isn't very useful information, but neither is user. I'm thinking that the ID should be replaced with a placeholder string along the lines of some user ID, and a comment explaining that a user/member object are also usable.

@Gawdl3y Gawdl3y merged commit 1273bb4 into discordjs:master Feb 19, 2017
@jamesandrewclarke jamesandrewclarke deleted the doc-fixes branch February 22, 2017 19:21
devsnek pushed a commit to devsnek/discord.js that referenced this pull request May 14, 2017
* Improve ban/unban examples

* Fix example comments

* Replace nondescript 'user' parameter with 'some user ID'

* Update Guild.js

* Update Guild.js
devsnek pushed a commit to devsnek/discord.js that referenced this pull request May 14, 2017
* Improve ban/unban examples

* Fix example comments

* Replace nondescript 'user' parameter with 'some user ID'

* Update Guild.js

* Update Guild.js
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.

7 participants