-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
WIP: move to the maubot plugin framework #23
Conversation
check for the folling headers: * `X-Gitlab-Token`` * `Content-Type` check for the following query parameterts: * `room`
gitlab/__init__.py
Outdated
self.app = web.Application() | ||
self.app.add_routes([web.post(self.config['path'], self.post_handler)]) | ||
|
||
self.runner = web.AppRunner(self.app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there isn't any special requirement for a separate web server, it should probably default to using the main maubot server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i add the option to use a custom webserver if a port is specified in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not needed, but I guess it can be an option if you want to add it
gitlab/gitlab_hook.py
Outdated
self.project = GitlabProject(body['project']) | ||
self.repository = GitlabRepository(body['repository']) | ||
self.commits = [GitlabCommit(commit) for commit in body['commits']] | ||
self.total_commits_count: int = int(body['total_commits_count']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff seems the exact use case for attrs (already a dependency) along with mautrix-python's SerializableAttrs. I think filter.py is a fairly good simple example on how that works.
Basically, you just make a @dataclass
(from attr import dataclass
) that also extends SerializableAttrs['YourClassName']
(I'm not sure if ['YourClassName']
could be omitted, type hints are complicated). After that, you simply define the fields as class properties with type hints and use YourClassName.deserialize(data_dict)
and data_dict = your_class_instance.serialize()
.
It's also fine if that's too complicated/not documented enough, I can change this to use that later myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to define a conversion funktion for the attrs?
I would like to convert the timestamps into datetime objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you just need to define a type hint (e.g. just set it to datetime.datetime
) and then add a serializer and deserializer for that type, something like this:
from datetime import datetime
from attr import dataclass
from mautrix.types import JSON, SerializableAttrs, serializer, deserializer
@serializer(datetime)
def datetime_serialize(dt: datetime) -> JSON:
return int(dt.timestamp())
@deserializer(datetime)
def datetime_deserialize(data: JSON) -> datetime:
return datetime.fromtimestamp(data)
@dataclass
class YourData(SerializableAttrs['YourData']):
...
ts: datetime
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should work with now. I am thinking about adding types for urls and so on so that one doesn't have str everywhere but that could be overkill
the path is not configurable at the moment as the `@wed` decorator can't evaluate labda funktions needs maubot/maubot#67
gitlab/gitlab_hook.py
Outdated
class GitlabChange(SerializableAttrs['GitlabChange']): | ||
|
||
previous: Union[GitlabLabel, str, int] | ||
current: Union[GitlabLabel, str, int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the deserializer currently doesn't support arbitrary unions, only Union[type, None]
(aka Optional[type]
). I could probably make it support arbitrary unions though if gitlab really sends different types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes gitlab sends different types and is inconsistent everywhere. at the moment it doesn't brake anything as it seems to be parsed as a mautrix.client.api.types.util.obj.Obj
(for a label change that is) but it would probably be nice to have the possibility of parsing Unions
+ add `!gitlab server login` + add `!gitlab server logout` + add `!gitlab server list` + enable db usage + rename module to `gitlab_matrix` a plugin that also uses the name `gitlab`
@tulir I have a problem with the database integration. basically i want to add multi-server support and implement a default server, so that you don't have to specify a server every time. I want to implement it as another table ( Can you help me with that? |
the bot should now have the same functionality as the old one written in go @tulir can you review the new code? |
ping @tulir |
Implemented Features:
needs maubot/maubot#72
Fixes #26
Fixes #25
Fixes #17
Fixes #14
Fixes #11
Fixes #10
Fixes #9
Fixes #1