search

HullSeals / HalpyBOT

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

[RELEASE] 1.6.0 #276

Closed Rixxan closed 1 year ago

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jun 13, 2022, 20:38

Added:

Changed:

Removed:

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jun 13, 2022, 20:38

requested review from @Rixxan, @rik079, and @stuntphish

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jun 20, 2022, 20:09

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 4, 2022, 15:00

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 4, 2022, 20:13

marked this merge request as ready

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 4, 2022, 21:04

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 5, 2022, 01:52

Commented on CLI/BackupFactUpdater/main.py line 80

error: casting an exception to a string

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 5, 2022, 01:52

Commented on halpybot/commands/drill.py line 159

Returning string errors from non-command handlers. This is a dangerous and slippery slope that incurs significant technical debt. I must strongly advise refactoring.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 5, 2022, 01:52

Commented on halpybot/commands/drill.py line 154

I wish python had rust's pattern matching capabilities, would lead to far simpler error handling code.

With that being said, I think its dangerous to depend upon the informal string cast of an exception object for dictating code paths. Consider refactoring to have more specific exception subclasses instead.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 5, 2022, 01:52

Commented on halpybot/commands/fact.py line 53

String concatenation inside an fstring. Nested fstring.

This should probably read

        f"Language: {langcodes[lang.lower()]} ({fact.language})'}\n"
Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:18

Commented on halpybot/commands/drill.py line 154

changed this line in version 5 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:18

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:19

Commented on halpybot/commands/fact.py line 53

changed this line in version 6 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:19

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:32

Commented on CLI/BackupFactUpdater/main.py line 80

changed this line in version 7 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:32

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:34

Commented on halpybot/commands/fact.py line 53

Fixed. How the heck did that get there.

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:34

Commented on halpybot/commands/drill.py line 154

Fixed. Added the NoNearbyEDSM which we made but forgot to apply. I think this resolves this problem.

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:34

Commented on halpybot/commands/drill.py line 159

Can you elaborate more on what you mean?

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 5, 2022, 19:34

Commented on CLI/BackupFactUpdater/main.py line 80

The CLIs are in need of a good overhaul and are ancillary programs attached to HalpyBOT that have not received the same level of polish as the core. While we want to fix any gaping security holes, these programs are not generally run very frequently. As such, while work should go into them, more issues other than critical program flaws should be held until 1.6.1.

I've pushed a quickfix for this one, however.

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 6, 2022, 19:31

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 6, 2022, 21:44

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 22:04

Commented on halpybot/commands/drill.py line 159

Returning errors as strings instead of throwing them is a footgun that you will seriously come to regret later on. its massive source of technical debt and can prove difficult to refactor out later.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/edsm.py line 175

Kill this. kill this with fire

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/edsm.py line 178

kill this with fire.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/fact.py line 192

Surely this should already be a string or does strip_non_ascii do something wierd?

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/forcejoin.py line 46

    if args[1] not in config["Force join command"]["joinable"].casefold():
Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/notify.py line 145

This is most probably incorrect. There should probably be a single instance of this NotificationLock object, which this command would need acquire.

As written, this lock probably provides no synchronization at all.

Secondarially, you should be using an async lock, otherwise when properly implemented you're gunna be thread blocking which doesn't work in async.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/notify.py line 181

See previous. This is not correct.

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/notify.py line 237

I recommend implementing __aenter__ and __aexit__ as well to make this an async context manager. see also: https://docs.python.org/3//library/contextlib.html#contextlib.asynccontextmanager

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/notify.py line 241

pretty sure this doesn't actually DO anything. You are also not handling any exception raised by the context body (see secondary arguments)

Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/ping.py line 98

Not sure if what you have here is actually valid, but the following certainly is.

    except aiohttp.ClientError as e:
        logger.exception("Error in Elite Server Status lookup.")
        raise EDSMConnectionError(
            "Unable to verify Elite Status, having issues connecting to the Elite API."
        ) from e
Rixxan commented 1 year ago

In GitLab by @theunkn0wn1 on Jul 6, 2022, 23:04

Commented on halpybot/commands/time.py line 34

String concat is evil.

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:13

Commented on halpybot/commands/fact.py line 192

Yes, it is always going to be a string at this point. However, for database-related reasons you'd have to ask Kenny about, the fact system only supports ASCII-characters. This function just makes sure no one can throw a wrench in the works with a corresponding outcome.

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:16

Commented on halpybot/commands/forcejoin.py line 46

@Rixxan I can't approve these, could you either enable that or implement it yourself?

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:40

Commented on halpybot/commands/notify.py line 145

Can't we just base this off an asyncio.Lock with added timer functionality? Would that solve all issues described?

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:41

Commented on halpybot/commands/notify.py line 181

https://gitlab.com/hull-seals/code/irc/halpybot/-/merge_requests/135#note_1018473206

Discussion here

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:41

Commented on halpybot/commands/notify.py line 237

https://gitlab.com/hull-seals/code/irc/halpybot/-/merge_requests/135#note_1018473206

Discussion here

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:43

Commented on halpybot/commands/notify.py line 241

It certainly does work, at least under normal circumstances. Your point is valid regardless, closing this one as it's sufficiently covered in other comments.

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:44

Commented on halpybot/commands/ping.py line 98

Will approve once Gitlab allows me to

Rixxan commented 1 year ago

In GitLab by @rik079 on Jul 7, 2022, 12:56

Commented on halpybot/commands/time.py line 34

from timeit import timeit

print(timeit("'It is currently ' + current_utc + ' UTC on ' + current_monthday + ', ' + year",
             setup="current_utc, current_monthday, year = '12:34', 'July 7', '2022'"))

> 0.167991

print(timeit("f'It is currently {current_utc} UTC on {current_monthday}, year {year}'",
             setup="current_utc, current_monthday, year = '12:34', 'July 7', '2022'"))

> 0.09389919999999996

That doesn't need any commentary I suppose :smile:

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 13:00

Commented on halpybot/commands/forcejoin.py line 46

changed this line in version 10 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 13:00

Commented on halpybot/commands/ping.py line 98

changed this line in version 10 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 13:00

added 1 commit

Compare with previous version

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/edsm.py line 178

changed this line in version 11 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/drill.py line 159

This diff is out of date. That issue was indeed fixed with 4a1671a1.

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/edsm.py line 175

Unsure what is being killed with fire - didn't we remove the offending str(er) here?

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/edsm.py line 178

Consider it killed.

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/time.py line 34

String concat effectively banished o7

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

Commented on halpybot/commands/time.py line 34

changed this line in version 11 of the diff

Rixxan commented 1 year ago

In GitLab by @Rixxan on Jul 7, 2022, 22:50

added 1 commit

Compare with previous version