Skip to content

Check if channel exists before referencing it#342

Closed
nnsee wants to merge 3 commits into
python-discord:masterfrom
nnsee:invalid-channel-fix
Closed

Check if channel exists before referencing it#342
nnsee wants to merge 3 commits into
python-discord:masterfrom
nnsee:invalid-channel-fix

Conversation

@nnsee
Copy link
Copy Markdown
Contributor

@nnsee nnsee commented Jan 31, 2020


name: Check if channel exists before referencing it
about: Avoid referencing #invalid-channel
closes: #335


Pull Request Details

Please ensure your PR fulfills the following criteria:

  • Have you joined the PythonDiscord Community?
  • Were your changes made in a Pipenv environment?
  • Does flake8 pass (pipenv run lint)

Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

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

LGMT

Copy link
Copy Markdown
Contributor

@Numerlor Numerlor left a comment

Choose a reason for hiding this comment

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

The constant which provides the channel list should be changed here instead of checking it against the server every time
https://github.com/python-discord/seasonalbot/blob/2679849160f160d8f70c597636798df185873523/bot/constants.py#L141-L146
The invalid channel could also be removed from the Channels constants class

@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Feb 1, 2020

The constant which provides the channel list should be changed here instead of checking it against the server every time
https://github.com/python-discord/seasonalbot/blob/2679849160f160d8f70c597636798df185873523/bot/constants.py#L141-L146

The invalid channel could also be removed from the Channels constants class

Aye, that was my initial reaction as well, however I opted to also check every time as an extra precaution as it seems like this would prevent the issue from popping up in the future (should the channel list change). I've removed the invalid channel #devtest from both Channels and WHITELISTED_CHANNELS.

@Numerlor
Copy link
Copy Markdown
Contributor

Numerlor commented Feb 1, 2020

Not sure what the consensus here is about checking it, because the channels don't really change much.
But if the check remains some sort of logging to notify about invalid channels so they can be removed could also prove to be useful.

@kwzrd
Copy link
Copy Markdown
Contributor

kwzrd commented Feb 22, 2020

I think I like the idea of just removing the invalid channel (i.e. ba69450).

... it seems like this would prevent the issue from popping up in the future (should the channel list change) ...

The underlying problem is a misconfigured / outdated constant. The #deleted-channel showing up is a symptom, and dd6063f silences it rather than fixes it. If there exists a problem like this in the code base, it should be observable so that it can be fixed.

@sco1
Copy link
Copy Markdown
Contributor

sco1 commented Feb 23, 2020

The underlying problem is a misconfigured / outdated constant. The #deleted-channel showing up is a symptom, and dd6063f silences it rather than fixes it. If there exists a problem like this in the code base, it should be observable so that it can be fixed.

We've come to an agreement on this, and rather than flagging this randomly when users bounce off of a whitelist, it would be better to solve this issue by validating the channel constants on bot startup & logging a warning if any of them fail to resolve so we can identify & update the problematic constant(s).

@sco1 sco1 added the status: waiting for author Waiting for author to address a review or respond to a comment label Feb 25, 2020
@sco1
Copy link
Copy Markdown
Contributor

sco1 commented Mar 2, 2020

Hi @neonSea, any interest in updating the implementation here?

@nnsee nnsee requested a review from a team as a code owner March 2, 2020 15:23
@nnsee
Copy link
Copy Markdown
Contributor Author

nnsee commented Mar 2, 2020

I will implement functionality to check for invalid channels on startup and warn if there are any. Should I create a separate PR for that, or is this one fine?

@nnsee nnsee closed this Mar 2, 2020
@Numerlor Numerlor mentioned this pull request Apr 3, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: waiting for author Waiting for author to address a review or respond to a comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.aoc leaderboard #deleted-channel

5 participants