Skip to content
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

refactor into modules #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor into modules #53

wants to merge 2 commits into from

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Oct 21, 2019

reading through the code i saw some opportunities to refactor. let me know what you think. the main idea is that i wanted to separate the quart app setup/controllers, from the github event handling code. i feel that it's a bit easier to read when it's split up this way. i got carried away and also decided it might be helpful to create a module that contains just messages that will get sent to users.

@webknjaz webknjaz requested a review from njsmith October 21, 2019 18:12
@webknjaz webknjaz added the enhancement New feature or request label Oct 21, 2019
return s


invite_message = _fix_markdown(
Copy link
Member

Choose a reason for hiding this comment

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

Could as well just have a Jinja template instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point! I would like to add that in as well.

snekomatic/messaging.py Outdated Show resolved Hide resolved

from .db import PersistentStringSet
from .gh import GithubApp
from .events import github_app
Copy link
Member

Choose a reason for hiding this comment

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

It's not "events", it's

Suggested change
from .events import github_app
from .event_handlers import github_app

conceptionally.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we might want the name to include "github" too, since we might have other kinds of events later?

I also guess we might eventually want to refactor along the lines of logically-related-features, like "inviting new users", "dependency bumping", etc., instead of lower-level functional categories like "reacting to github events", "text strings", etc.

But no need to make the perfect the enemy of the good... I'm happy with merging this now and then worrying about that stuff later. @wgwz, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like both of the suggestions! I definitely prefer taking it one step further and splitting up the modules into logically related features. I'm willing to do that for this MR.

Co-Authored-By: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants