HullSeals / HalpyBOT

The Hull Seals IRC Utility bot, built with Pydle
GNU General Public License v3.0
7 stars 3 forks source link

[Feature] Add additional processing to EDSM - [merged] #279

Closed Rixxan closed 1 year ago

Rixxan commented 2 years ago

Merges feature/edsm-malformed-processor -> develop

This helps HalpyBOT tolerate and process a malformed response from EDSM. This is useful to help specify down where an error is occurring with EDSM in the event of a malformed response, as seen with Abildgaard Jadrake.

Rixxan commented 2 years ago

requested review from @rik079

Rixxan commented 2 years ago

added 1 commit

Compare with previous version

Rixxan commented 2 years ago

added 1 commit

Compare with previous version

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 94

lolwut

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 249

This case doesn't log an exception but others do?

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 208

other exception handlers log the exception, but this doesn't?

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 151

other exception handlers log the exception, but this doesn't?

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 101

other exception handlers log the exception, but this doesn't?

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Jul 31, 2022, 17:16

Commented on halpybot/commands/edsm.py line 348

other exception handlers log the exception, but this doesn't?

Rixxan commented 2 years ago

added 1 commit

Compare with previous version

Rixxan commented 2 years ago

I think this is referencing old code you said was bad and we added the comment but refactored the badness

Rixxan commented 2 years ago

changed this line in version 5 of the diff

Rixxan commented 2 years ago

added 1 commit

Compare with previous version

Rixxan commented 2 years ago

Added a lot of new exception logging lines in 7e064158. Let me know if that resolves the issue.

Rixxan commented 2 years ago

resolved all threads

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Aug 6, 2022, 24:01

Commented on halpybot/commands/fact.py line 73

I would be extra warey here, especially since its user input. My paranoia makes me want to pass the maxsplit=1 parameter to the str.split call. YMMV (you may mark this as resolved with no action)

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Aug 6, 2022, 24:01

Commented on halpybot/commands/fact.py line 116

this block of code is duplicated over at least 4 command handlers. consider refactoring.

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Aug 6, 2022, 24:01

Commented on halpybot/packages/announcer/announcer.py line 104

I question if the exception needs to be logged here, especially since you then re-raise the exception. in other words, logging becomes the caller's problem.

Rixxan commented 2 years ago

In GitLab by @theunkn0wn1 on Aug 6, 2022, 24:01

Commented on halpybot/packages/announcer/announcer.py line 101

Conversely, you probably want logging here, so you can have something to work with when someone comes screaming that the twitter stuff doesn't work

Rixxan commented 2 years ago

Out of scope, will move to own issue (#140).

Rixxan commented 2 years ago

Out of scope, will move to own issue (#140).

Rixxan commented 2 years ago

changed this line in version 6 of the diff

Rixxan commented 2 years ago

added 1 commit

Compare with previous version

Rixxan commented 2 years ago

resolved all threads

Rixxan commented 2 years ago

In GitLab by @rik079 on Aug 10, 2022, 16:41

approved this merge request

Rixxan commented 2 years ago

In GitLab by @rik079 on Aug 10, 2022, 16:41

mentioned in commit ac11385437f3a50a610bec1997b4398b38e5e921