[WIP] refactor, new features and bugfixes #387

rbignon · 2023-01-09T00:17:29Z

New features:

display more information at connection

Welcome to the Slack Server Teamname, romain!
Your host is domain.slack.com, running version localslackirc-2.0
There are 125 users and 30 bots on 1 server
5 Slack Workspace Admins
105 channels formed

rename user's nickname at connection if not using the right one

You're now known as romain

use the slack domain (*.slack.com) instead of gethostname for server name and user hostnames
more information in WHOIS:

romain [U03D36HJC14@domain.slack.com]
 ircname  : Romain Bignon
          : Developer
 account  : romain@example.org
 account  : +33123456789
 account  : https://avatars.slack-edge.com/2022-10-27/XXXX_original.png
 channels : #general #random
 server   : domain.slack.com [TeamName]
 away     : My Status
          : Workspace Owner
 idle     : 0 days 23 hours 39 mins 19 secs
End of WHOIS

if I send a message to a user who is away, I get a RPL_AWAY message
add --thread-replies parameter to display thread answers in IM/channels instead of creating a new channel
handle threads in IM
correctly handle edited/deleted messages in IM
fix /me messages
USERHOST displays away/admin flags
handle CAP command (currently empty)
handle JOIN : command sent by recent irssi versions during connection (I don't know why, but if it doesn't get the right answer it freezes)
fix crash in some case during shutdown or user disconnection
handle slack connection errors
handle MODE with parameters to display an error
handle MODE +b to display an emply banlist
add command WHOWAS
concatenate multilines topic
use colored logs
handle groups and MPIM join/parts

Internal changes:

move *.py files into a new localslackirc module (requires to use python -m localslackirc to run it locally, it will need to change packaging to create a localslackirc script)
split irc.py into daemon.py and irc.py
do not use bytes anymore
rework of the commands handling
use logging module instead of syslog
do not crash if an exception is uncatched in a command handler, just display an error to user and display the backtrace in logs
flake8 fixes

rbignon · 2023-01-09T00:22:10Z

I split into several commits:

create localslackirc python module and move python files into does not change anything
lot of changes is a commit with things not yet split
move methods inside the Server class is only method moves, no code changes

ltworf · 2023-01-09T16:09:51Z

Hello,

it's too big of a change to be done in a single PR.

As it is, tests are failing
There are regressions (logging to journald? deb package?)
There are changes that I can't understand why they are done and commit message offers no explaination.

I'm not opposed to merging big refactors, but they must be complete and not break anything.

Please make different changes into different branches so that I can respond and evaluate them.

ltworf · 2023-01-09T16:11:57Z

Basically every point should probably be a separate pull request, of one or many commits, depending how big or deep the change is.

At the very least existing tests should be passing.

rbignon · 2023-01-09T17:44:16Z

Hello,

it's too big of a change to be done in a single PR.

As it is, tests are failing

There are regressions (logging to journald? deb package?)

There are changes that I can't understand why they are done and commit message offers no explaination.

I'm not opposed to merging big refactors, but they must be complete and not break anything.

Please make different changes into different branches so that I can respond and evaluate them.

As said, the commit 'lot of changes' is not yet split. Also I've not yet fix packaging and tests (for packaging it would be nice if you could help me), that's a draft.

It's standard to log with the logging python package, it writes on stderr, journald reads stderr.

ltworf · 2023-01-10T02:06:13Z

It's standard to log with the logging python package, it writes on stderr, journald reads stderr.

I know how it works. But it also discards log level information, which gets lost forever. The log module has a syslog driver that needs to be used in order to preserve this information.

rbignon · 2023-01-10T10:40:19Z

It's standard to log with the logging python package, it writes on stderr, journald reads stderr.

I know how it works. But it also discards log level information, which gets lost forever. The log module has a syslog driver that needs to be used in order to preserve this information.

try:
    from systemd.journal import JournalHandler
except ImportError:
    pass
else:
    logging.root.addHandler(JournalHandler())

Something like that would be ok? I guess if you want to let people configure their log handlers, you should let them give a logging.yml config file, there is no reason to support syslog only.

ltworf · 2023-01-10T11:55:35Z

That's an external dependency… could be done but I'm not too happy about those.

Anyway I'm ok with extra configuration… logging.yml makes no sense because it's not how the rest is configured. Default behaviour has to remain unchanged if possible.

rbignon · 2023-01-10T12:04:14Z

I don't understand why using syslog instead of journald for an application. But if you want I can add an extra parameter --syslog/SYSLOG to add the syslog handler.

It fixes ME/edit/delete notification in IM

It crashed if a user deletes a message in a thread where the first message was already deleted

Slack.tload log errors

Also, handle user_change in the common way instead of a custom condition. 'message.message_replied' was not handled, as it's a subtype of 'message', we can't ignore it, so handle as a dataclass but ignore it.

ltworf · 2023-01-11T15:49:37Z

journald is only present with systemd. syslog is way more widespread.

If we don't use any specific journald features, there is no reason to require an external dependency.

I'd like the behaviour to remain the same: always log to syslog, and log to terminal if it's running in a terminal. No extra option should be required just to switch to the logging module

…nels twice

Ignore these errors: - E501 line too long - W504 line break after binary operator

rbignon · 2023-01-22T15:54:45Z

I fixed tests, maybe you can help me to change packaging?

ltworf · 2023-01-22T16:23:57Z

Hello, thanks for this.

I'm going to have to ask you to split this into multiple pull requests, "title and phone in profiles" should not be in the same as "coloured logs".

After that I can review/merge/reject on the changes.

ltworf · 2023-01-22T16:29:50Z

If it's easier for you you can just post the pull requests one by one, if the later commits are already based on the previous ones.

rbignon · 2023-01-22T17:45:32Z

Ok, but it'll be complicated if you require to edit a commit or to reject ones.

ltworf · 2023-01-22T18:02:18Z

I can either reject the whole thing or review and decide case by case though… as it is, there could be anything in there.

rbignon · 2023-01-22T18:14:14Z

Done.

rbignon · 2023-01-22T18:15:24Z

Maybe, before reviewing separate PR or commit, pull all commits and read the whole code.

rbignon · 2023-01-22T18:47:03Z

Ok I don't care to be merged into your repository, I maintain my own fork.

Bye.

ltworf · 2023-01-22T20:12:44Z

Maybe, before reviewing separate PR or commit, pull all commits and read the whole code.

As it is, the whole code, can't be merged and has several issues.

Since you put everything into one single branch, the issues are too many to even list all of them.

Having separate independent branches would have made it possible to comment on the issues.

Ok I don't care to be merged into your repository, I maintain my own fork.

Ok. No problem.

rbignon added 5 commits January 8, 2023 21:23

remove the localslack irc script

2b0da4d

create localslackirc python module and move python files into

8c2135b

use logging python module instead of syslog

2da6f05

Slack: use explicit close() method instead of destructor

d2d87af

daemon.py: application module

23f0bc9

rbignon changed the title ~~Wip refactor~~ [WIP] refactor, new features and bugfixes Jan 9, 2023

rbignon force-pushed the wip-refactor branch from b38e43c to 63c9143 Compare January 9, 2023 17:36

rbignon added 15 commits January 10, 2023 18:05

lot of changes

7d803b1

move methods inside the Server class

06d3d00

WHOIS: display common channels

8a42460

IRC commands: catch exceptions and display a message to user

2f81df3

WHOWAS: add command (used by irssi if the user is not found with WHOIS)

865ce46

handle title and phone in Profile

7fa088a

WHOIS: display status in the away message

6ba0224

PRIVMSG: display status text if the user is away

3b25c45

SENDFILE: handle if the file doesn't exist

8cd80ab

WHOIS: rework lines order

eb3c724

move private messages sender substitution in IRC Server

e507f81

It fixes ME/edit/delete notification in IM

fix NOSUCHCHANNEL and NUSUCHNICK messages

5362b79

Slack.get_members accept a new parameter 'refresh'

ceb2fef

Slack.channels can take True, False and None values for refresh

17e9fa4

Slack.get_user_by_name: if the user cache is empty, fetch users

5faee70

rbignon added 7 commits January 10, 2023 18:05

change way to handle bot messages

0b138e6

use colored logs

3e2add8

handle ConnectionResetError

0514fe0

fix several issues when handling threads

4e58a80

It crashed if a user deletes a message in a thread where the first message was already deleted

TOPIC: when the topic changed, concatenate multilines

952a5ef

handle TypedloadValueError instead of all exceptions

5a88c32

Slack.tload log errors

Slack.event is now a generator

f5dda16

Also, handle user_change in the common way instead of a custom condition. 'message.message_replied' was not handled, as it's a subtype of 'message', we can't ignore it, so handle as a dataclass but ignore it.

rbignon added 3 commits January 22, 2023 15:28

fix description of --thread-replies

4e26811

add Channel.irc_name property

6614b88

handle leaving channels and group/MPIM channels, and do not join chan…

1fff714

…nels twice

rbignon force-pushed the wip-refactor branch from 63c9143 to 1fff714 Compare January 22, 2023 14:33

rbignon added 4 commits January 22, 2023 15:41

fix tests

fba4086

add a syslog handler

c6bbb8d

remove unused rocker module

f626119

flake8 --ignore=E501,W504

2f03ac4

Ignore these errors: - E501 line too long - W504 line break after binary operator

ltworf closed this Jan 22, 2023

rbignon deleted the wip-refactor branch January 29, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] refactor, new features and bugfixes #387

[WIP] refactor, new features and bugfixes #387

rbignon commented Jan 9, 2023 •

edited

rbignon commented Jan 9, 2023

ltworf commented Jan 9, 2023

ltworf commented Jan 9, 2023

rbignon commented Jan 9, 2023

ltworf commented Jan 10, 2023

rbignon commented Jan 10, 2023

ltworf commented Jan 10, 2023

rbignon commented Jan 10, 2023

ltworf commented Jan 11, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

ltworf commented Jan 22, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

rbignon commented Jan 22, 2023

rbignon commented Jan 22, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

[WIP] refactor, new features and bugfixes #387

[WIP] refactor, new features and bugfixes #387

Conversation

rbignon commented Jan 9, 2023 • edited

rbignon commented Jan 9, 2023

ltworf commented Jan 9, 2023

ltworf commented Jan 9, 2023

rbignon commented Jan 9, 2023

ltworf commented Jan 10, 2023

rbignon commented Jan 10, 2023

ltworf commented Jan 10, 2023

rbignon commented Jan 10, 2023

ltworf commented Jan 11, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

ltworf commented Jan 22, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

rbignon commented Jan 22, 2023

rbignon commented Jan 22, 2023

rbignon commented Jan 22, 2023

ltworf commented Jan 22, 2023

rbignon commented Jan 9, 2023 •

edited