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

[WIP] Refactor to enable unit testing #285

Closed
wants to merge 1 commit into from

Conversation

ValerianClerc
Copy link
Member

Description

Tackling #268. This is a WIP pull request for easier viewing of changes + comments.

Motivation and Context

Currently the discord.py API is tightly coupled with the custom logic for each command. This makes writing unit tests for that logic very difficult. By separating the command logic from the discord.py API, we can write tests for our functions.

How Has This Been Tested?

Manually so far, this is tentative. Still iterating on design of the refactor before fully cleaning this up.

Changes:

  • Currently, the only relevant files to look at are cogs/memes.py and cogs/controllers/memes.py because I don't wanna waste time refactoring everything until I know that the idea is good.
  • I was thinking of doing a decorator because that the cleanest option, but some of the functions require different ctx actions (like bac and mix need ctx.message.delete, while some of the simpler ones just use ctx.message.send)
  • Therefore, instead I just factored out the discord.py command from the actual logic/computation. The discord.py commands are going to be in cogs/ while the "controllers" (we can think of a better name) that handle the logic go in cogs/controllers

@idodin
Copy link
Member

idodin commented Mar 28, 2020

If I understood this correctly - to sum up:

  • cogs/[name].py will contain the actual endpoint definitions of our commands and drive logic in the cogs/controllers/[name].py module.

This change seems fine to me, although it complicates development of new features a bit. All in all, I think it may modularize code a bit more nicely - but we might want to layout conventions for this architecture (i.e. 1-to-1 mapping between cog and controller or do we separate concerns even more in the controller package?)

Also (I know you've already said its a placeholder) but can we find a better name for this than Controller - Controllers tend to act as the actual endpoint e.g. in an MVC. Would Service be a more appropriate name?

@ValerianClerc
Copy link
Member Author

@idodin Yes! You've understood it 100% I think.

I like the name "Service" a lot, I'll roll with that for now!

And yeah I agree with your thought, this will complicate the structure a bit. But ideally we'll document it well and it'll be a clear separation of concerns, as well as being easily testable? I'm definitely open to suggestions.

I was talking with David and Johanan, and we were thinking that with this refactor we could look into embracing a more functional approach. And then we can reuse these Service component functions as building blocks to make up future functions?

Let me know what you think!

@idodin
Copy link
Member

idodin commented Apr 1, 2020

@ValerianClerc Sounds good to me - also agree on being a bit more functional, but I think we'd need to modularize more if we wanted to use functions as building blocks for more complex logic. That being said, let me know if you need help with testing!

@jidicula jidicula removed their request for review October 13, 2020 19:23
@davidlougheed
Copy link
Member

closed for now until we get a better grasp on what we're planning with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants