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

prepare v5.1.0 #1233

Merged
merged 23 commits into from
Jul 19, 2022
Merged

prepare v5.1.0 #1233

merged 23 commits into from
Jul 19, 2022

Conversation

idinium96
Copy link
Member

@idinium96 idinium96 commented Jul 18, 2022

New stuff

  • ✨ Add !links command (#1236)

Updates/Changes

  • ✅ Properly handle inventory fetch (#1228)
  • ✅ Allow Discord bot to chat with non-admin (#1234, a39cf2c, 483dbbe)
    • ✨ Add your bot to the TF2Autobot Discord server! Read: wiki
  • 🛑 Throw an error if Rep.tf returns invalid response message (#1235)
  • 🔄 Update !stock command to include item assetids (#1237)

Fixes

  • 🐛 Fix html not filtered in log (v5.0.1 extension, dce09f9, 632793b)

@RobotoLev
Copy link
Collaborator

RobotoLev commented Jul 18, 2022

Just saw your review request on #1234. Since it was already merged, will leave it here.

As I understand right now, steamID.type === 0 is being used when we work with quasi-id (which doesn't have actual steam ID). Probably it's good. But I don't understand, do you allow !message for Discord non-admin user: on one side, you're blocking it here, but on the other hand, you mentioned it in help.

Also I see two problems with code quality, which I can fix:

  • The try-catch duplication here can be easily simplified.
  • I have a feeling that this part requires some logging.

P.S.: Last part doesn't change any functionality, so it's possible to make refactor after the release (I'm just not sure when I will have time to do it)

@RobotoLev
Copy link
Collaborator

Also, when I was implementing redirectAnswerTo thing, I thought that it might be used for something other in the future, not just for Discord. That's why I inserted checking for it being instance of DiscordMessage (Message from discord.js). I would make something similar here. Right now it's not required, but might be useful in the future.

@idinium96
Copy link
Member Author

Thank you for the review.

Just saw your review request on #1234. Since it was already merged, will leave it here.

As I understand right now, steamID.type === 0 is being used when we work with quasi-id (which doesn't have actual steam ID). Probably it's good. But I don't understand, do you allow !message for Discord non-admin user: on one side, you're blocking it here, but on the other hand, you mentioned it in help.

Oh yeah, I think I accidentally include !message together with other available commands. I will remove it (or you can for sure, just create another pull request for it).

Also I see two problems with code quality, which I can fix:

  • The try-catch duplication here can be easily simplified.

Oh right, just combine altogether. 👍

  • I have a feeling that this part requires some logging.

Not sure why need some logging. This part will just to make sure to ignore command spam on Discord, as far as I understand.

@idinium96
Copy link
Member Author

Also, when I was implementing redirectAnswerTo thing, I thought that it might be used for something other in the future, not just for Discord. That's why I inserted checking for it being instance of DiscordMessage (Message from discord.js). I would make something similar here. Right now it's not required, but might be useful in the future.

Yeah, for now, maybe just stick with this.

@idinium96
Copy link
Member Author

Merging...

@idinium96 idinium96 merged commit 7410258 into master Jul 19, 2022
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