-
Notifications
You must be signed in to change notification settings - Fork 112
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
Battlemetrics Upcoming Wipe v2 #305
base: master
Are you sure you want to change the base?
Battlemetrics Upcoming Wipe v2 #305
Conversation
+1 be merged ? |
I will test the PR tomorrow |
Awesome let me know if i should fix something :) |
@@ -719,18 +753,20 @@ module.exports = { | |||
fields: [ | |||
{ name: playersFieldName, value: `\`${rustplus.getCommandPop(true)}\``, inline: true }, | |||
{ name: timeFieldName, value: `\`${time[0]}\``, inline: true }, | |||
{ name: wipeFieldName, value: `\`${rustplus.getCommandWipe(true)}\``, inline: true }], | |||
time[1] ? { name: timeLeftTitle, value: `\`${time[1]}\``, inline: true } : emptyFieldValue, | |||
], | |||
timestamp: true | |||
}); | |||
|
|||
if (time[1] !== null) { |
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.
Why is this dependent on time?
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 switched field Time till
with Wipe
field so wipe info is on second row and Players
, Time
and Time till
are on first row. So this row you pointed is field Time till
(if you compare actual bot with images i add to pull request you will see).
If you want it in different order i can fix it however you like.
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.
What I dont understand is why you write out wipe stuff only if time[1] is present...
This part:
if (time[1] !== null) { embed.addFields( { name: wipeFieldName, value: "${rustplus.getCommandWipe(true)}", inline: true }, firstWipe, secondWipe, ); } else { embed.addFields(emptyFieldValue); }
src/util/CreateInstanceFile.js
Outdated
@@ -51,7 +51,8 @@ module.exports = (client, guild) => { | |||
server: null, | |||
event: null, | |||
team: null, | |||
battlemetricsPlayers: null | |||
battlemetricsPlayers: null, | |||
battlemetricsUpcomingWipes:null |
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.
Remove this
src/util/CreateInstanceFile.js
Outdated
@@ -153,7 +154,8 @@ module.exports = (client, guild) => { | |||
server: null, | |||
event: null, | |||
team: null, | |||
battlemetricsPlayers: null | |||
battlemetricsPlayers: null, | |||
battlemetricsUpcomingWipes:null |
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.
Remove this
src/languages/en.json
Outdated
@@ -766,5 +766,8 @@ | |||
"wipeDetected": "Wipe detected!", | |||
"yield": "Yield", | |||
"youAreAlreadyLeader": "You are already leader.", | |||
"youAreNotPairedWithServer": "Leader command does not work because you're not paired with the server." |
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.
phrases should be in order (alphabetical)
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.
Should consider adding server_rust_wipes to evaluation function. But from testing it looks like a few more elements have been added to the API. I could fix it.
src/discordTools/discordEmbeds.js
Outdated
if (upcomingWipes.length > 0){ | ||
const closestWipe = upcomingWipes[0]; | ||
if (closestWipe.type === 'map' || closestWipe.type === 'full') { | ||
// try match next map or full wipe |
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.
To make me happy, use /**/
@@ -531,6 +531,16 @@ module.exports = { | |||
style: enabled ? SUCCESS : DANGER | |||
})); | |||
}, |
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.
A little space please :D
I really like this addition, great work @Turtlak12345 !! |
@alexemanuelol Hopefully i fixed all. Evaluation for Battlemetrics, did i do it correctly? Also not sure if i should resolve comments because im used to that who have problem will resolve it when all is correct... |
src/languages/en.json
Outdated
@@ -765,6 +769,5 @@ | |||
"wipe": "Wipe", | |||
"wipeDetected": "Wipe detected!", | |||
"yield": "Yield", | |||
"youAreAlreadyLeader": "You are already leader.", | |||
"youAreNotPairedWithServer": "Leader command does not work because you're not paired with the server." |
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.
Dont remove "youAreNotPairedWithServer"
if (upcomingWipes.length > 0){ | ||
const closestWipe = upcomingWipes[0]; | ||
if (closestWipe.type === 'map' || closestWipe.type === 'full') { | ||
/* try match next map or full wipe*/ |
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.
Some indentation here please
@@ -667,6 +669,11 @@ class Battlemetrics { | |||
if (content['status'] === false) this.offlinePlayers.push(playerId); | |||
} | |||
|
|||
const rustWipes = details.rust_wipes; |
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.
Place this closer to the other evaluation checks, below the rustmap evaluation check instead of down here
@@ -801,6 +809,18 @@ class Battlemetrics { | |||
return ordered.map(e => e[1]); | |||
} | |||
|
|||
getUpcomingWipesOrderedByTime() { |
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.
Does this run even if there is a server that does not give any details about upcoming wipes? Or is rust_wipes always available in the battlemetrics API call?
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.
Also add a comment header for the function
a5d6956
to
881faa2
Compare
I remade showing of upcoming wipes based on discord and previous PR comments (#302)
There is no longer own embed showing all upcoming wipes but only closest wipe. If wipe got type
map
orfull
it will show as map or full wipe:If closest wipe is not type
map
orfull
or none of these types is found then closest wipe with any type will be shown (atm. can be typeunknown
)Whole functionality can be turn on/off in settings (default: off)