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

Add cases output in fun cog. #1457

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

JelmerKorten
Copy link
Member

Closes #707 if approved.
(new PR because of branch)

Description

  • Added 4 commands to fun.py to output a string in different cases to add to the already existing .randomcase
    PascalCase, camelCase, snake_case, SCREAMING_SNAKE_CASE
    .pascalcase aliases [.pascal .pcase]
    .camelcase aliases [.camel .ccase]
    .snakecase alias [.scase]
    .screamingsnakecase aliases [.screamsnake .ssnake .screamingsnake]
  • Added a helper function in utils/helpers.py to attempt to strip a string of punctuation and case.

Did you:

@JelmerKorten JelmerKorten requested a review from shtlrs February 16, 2024 18:25
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

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

Looks good & works well, thanks!

Copy link
Member

@hedyhli hedyhli left a comment

Choose a reason for hiding this comment

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

I can see that pretty much all the functionalities are there, well done! I have a few comments.

bot/exts/fun/fun.py Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
bot/utils/helpers.py Outdated Show resolved Hide resolved
@JelmerKorten JelmerKorten requested a review from hedyhli February 29, 2024 16:25
Copy link
Member

@hedyhli hedyhli left a comment

Choose a reason for hiding this comment

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

Looks good and works well, just a few trivial comments.

cleaned_text, embed = await self._clean_text(ctx, text, conversion_func)
await ctx.send(content=cleaned_text, embed=embed)

@commands.command(name="screamingsnakecase", aliases=("screamsnake", "ssnake", "screamingsnake",))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@commands.command(name="screamingsnakecase", aliases=("screamsnake", "ssnake", "screamingsnake",))
@commands.command(name="screamingsnakecase", aliases=("screamsnake", "ssnake", "screamingsnake", "sscase"))

How about having a*case alias for all our case commands? It made sense to me during testing and was surprised it wasn't already added 😄

Comment on lines +10 to +11
def neutralise_string(txt: str | None) -> list[str] | None:
"""Attempts to neutralise all punctuation and cases and returns a string of lowercase words."""
Copy link
Member

Choose a reason for hiding this comment

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

The docstring would have to be updated to reflect the new function. Maybe the name too, up to you.

Suggested change
def neutralise_string(txt: str | None) -> list[str] | None:
"""Attempts to neutralise all punctuation and cases and returns a string of lowercase words."""
def split_words(txt: str | None) -> list[str] | None:
"""Neutralise all punctuation and cases and return a list of separated words."""

Comment on lines +94 to +97
text = helpers.neutralise_string(text)
return "_".join(
text
)
Copy link
Member

Choose a reason for hiding this comment

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

Consider making the variable name of the returned value more meaningful since the helper function was updated, and it's not immediately obvious that the text is actually a list here.

Suggested change
text = helpers.neutralise_string(text)
return "_".join(
text
)
words = helpers.neutralise_string(text)
return "_".join(words)

Note that this also applies to the conversion_func for other case commands :)

@wookie184 wookie184 added type: feature Relating to the functionality of the application. status: waiting for author Waiting for author to address a review or respond to a comment category: fun Related to fun and games labels Apr 14, 2024
@Xithrius
Copy link
Member

Xithrius commented Jul 5, 2024

@JelmerKorten Hello! Will you be able to finish up this PR? Thanks!

@JelmerKorten
Copy link
Member Author

JelmerKorten commented Jul 5, 2024 via email

@Xithrius
Copy link
Member

Xithrius commented Jul 5, 2024

Thanks for the quick response! No rush. If you need any help completing this PR, don't hesitate to ask.

@Xithrius Xithrius added the area: backend Related to internal functionality and utilities label Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to internal functionality and utilities category: fun Related to fun and games status: waiting for author Waiting for author to address a review or respond to a comment type: feature Relating to the functionality of the application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New .case commands
5 participants