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][Welcome] Add dynamic welcome image generation #582

Open
wants to merge 48 commits into
base: V3/testing
Choose a base branch
from

Conversation

jlacsamana
Copy link

@jlacsamana jlacsamana commented Sep 11, 2022

Summary

Implements the features requested in Issue #579 as requested.

Testing

Loading Cog
image

Creating Directory To Store Images (Has folders for each server)
Before
image

After
image

Ensure Directory To Store Images For Specific Server exists: ensureCurrentServerHasImgCache()
Before
image

image

After
image

Random Welcome Image Generation: generateRandWelcomeImg()
image
3 Passes of the the random image generator
image

Welcome Image toggle on and off: [p]welcomeset greetings toggleimg
When there is no cache created for this server, make one & toggle to false
image

If there are no images in cache, toggle to false, and alert user about it
image

If there are images in cache, and currently toggled false, toggle true
image

If there are images in cache, and currently toggled true, toggle false
image

Fetch Image Template: [p]welcomeset greetings image template
image

Add a new image: [p]welcomeset greetings image add
if an image is added with an unused name
image

if image is added, but name is already in use
image

if command is used, but no image is provided
image

if command is used, but multiple images are provided
image

Remove an existing image by name: [p]welcomeset greetings image remove
if name specified matches the name of an image in this server's cache
image

if name specified doesn't match the name of any image in this server's cache
image

if last image removed, toggles off random image generation
image

View an image, fetched from server's image cache by name: [p]welcomeset greetings image view
if name specified matches the name of an image in this server's cache
image

if name specified doesn't match the name of any image in this server's cache
image

Fetch the list of all the names of the images in this server's welcome image cache: [p]welcomeset greetings image list
If there are no images in the cache yet
image

When there are images in the cache
image

@jlacsamana jlacsamana linked an issue Sep 11, 2022 that may be closed by this pull request
@jlacsamana jlacsamana changed the base branch from V3/develop to V3/testing September 11, 2022 10:35
@jlacsamana jlacsamana marked this pull request as ready for review September 12, 2022 05:54
Copy link
Member

@quachtridat quachtridat left a comment

Choose a reason for hiding this comment

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

Could you exclude other files and only include cogs/welcome files in the PR? Let the changes be local to the issue you're fixing 👍

@abelathomas
Copy link
Member

Hola, do we have any updates on this @jlacsamana ?
Also please note that this is out of date due to the dpy2 update

Copy link
Member

@Injabie3 Injabie3 left a comment

Choose a reason for hiding this comment

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

Did a first pass on latest changes, please add screenshots of the testing done.

.gitignore Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
Copy link
Member

@quachtridat quachtridat left a comment

Choose a reason for hiding this comment

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

Noted a number of things. Also can you add type hints for your methods too? I see that command handling methods have type hints but not the rest.
There are a number of .vs files so please exclude them.

cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/constants.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
@Injabie3
Copy link
Member

Any updates on this one?

Copy link
Member

@quachtridat quachtridat left a comment

Choose a reason for hiding this comment

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

Did a quick pass. A few TODOs for you:

  • Resolve all the comments. There are still unresolved comments from past reviews without any replies.
  • Keep sentences in docstring and replies consistent. Some of them have multiple verb tenses and don't end with a dot or an exclamation mark.
  • Exclude irrelevant files. I see unnecessary .gitignore and .vs changes that get committed, but they don't contribute to the feature you're implementing. If you need to have your own .gitignore, you can add your own one with Git global setting core.excludesFile. Try to prepare your PR properly so that only files in cogs/welcome are included.

cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
cogs/welcome/welcome.py Outdated Show resolved Hide resolved
@quachtridat
Copy link
Member

Also I've got to break it to you...
I don't see the use of emojis helps with your commits. If your commits have to do with black reformatting, grammar fixes, etc., then please describe them accordingly, and vice versa, also try to keep your changes true to what is described by their respective commits. Even though PR commits get squashed before merging, commit messages are still important in explaining changes along the way.

Copy link
Member

@Injabie3 Injabie3 left a comment

Choose a reason for hiding this comment

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

Please remove all VS code additions from this PR.

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.

[Welcome] Add image generation using avatar
4 participants