Skip to content

Tag system #135

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

Merged
merged 12 commits into from
Oct 20, 2021
Merged

Tag system #135

merged 12 commits into from
Oct 20, 2021

Conversation

illuminator3
Copy link
Contributor

@illuminator3 illuminator3 commented Sep 18, 2021

This PR adds a tag system & commands to use it. Closes #61.

Implemented commands are:

  • /tag <id> to view a tag
  • /tags to view all tags

and management commands

  • /tag-manage raw <id> to view the raw contents of a tag
  • /tag-manage create <id> <content> to create a tag with given content
  • /tag-manage create-with-message <id> <message-id> to create a tag based on the content of an existing message
  • /tag-manage edit <id> <content> to edit a tag with given content
  • /tag-manage edit-with-message <id> <message-id> to edit a tag based on the content of an existing message
  • /tag-manage delete <id> to delete an existing tag

The with-message overloads for create and edit are necessary and super useful when having more complex content with code formatting and similar, since slash commands do not support newlines.

@illuminator3 illuminator3 added enhancement New feature or request new command Add a new command or group of commands to the bot priority: normal labels Sep 18, 2021
@illuminator3 illuminator3 requested review from a team as code owners September 18, 2021 11:47
@illuminator3 illuminator3 self-assigned this Sep 18, 2021
@illuminator3
Copy link
Contributor Author

Seems like I forgot to add javadoc, will add it now

Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

Well done.

While I would strongly prefer it if you made usage of sub-commands instead.
Makes it easier for the user and will allow a ton more commands.

This in total is 8 commands while it could be 1.

@illuminator3
Copy link
Contributor Author

illuminator3 commented Sep 18, 2021

While I would strongly prefer it if you made usage of sub-commands instead.

I first thought about that as well, but it would be kinda confusing for new users to do /tag view <id> instead of just /tag <id>. I could maybe do sub commands with a /tagmanage <create|edit|delete> or /managetag.

@Tais993
Copy link
Member

Tais993 commented Sep 18, 2021

While I would strongly prefer it if you made usage of sub-commands instead.

I first thought about that as well, but it would be kinda confusing for new players to do /tag view <id> instead of just /tag <id>. I could maybe do sub commands with a /tagmanage <create|edit|delete> or /managetag.

Do like the idea of a separate tagmanage command, even better than one for all

@illuminator3
Copy link
Contributor Author

While I would strongly prefer it if you made usage of sub-commands instead.

I first thought about that as well, but it would be kinda confusing for new players to do /tag view <id> instead of just /tag <id>. I could maybe do sub commands with a /tagmanage <create|edit|delete> or /managetag.

Do like the idea of a separate tagmanage command, even better than one for all

Yea, I'll implement it!

Tais993
Tais993 previously approved these changes Sep 18, 2021
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

I approve it.

Still I'll mention that it's maybe nice to replace some of the code in the switch with methods?
Just a suggestion, the code is good. 👍

Tais993
Tais993 previously approved these changes Sep 18, 2021
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

You're cute keep it up

@Tais993
Copy link
Member

Tais993 commented Sep 18, 2021

image
Make sure you're logged in when trying

Zabuzard
Zabuzard previously approved these changes Oct 16, 2021
Zabuzard
Zabuzard previously approved these changes Oct 17, 2021
@Zabuzard
Copy link
Member

Rebase on develop

@Zabuzard
Copy link
Member

Zabuzard commented Oct 17, 2021

NotNull found out that the MarkdownSanitizer.escape(...) does not correctly escape \\. It leaves it as \\ instead of making \\\\. We have to test whether the raw-command works with such input and whether its an actual issue in practice (thinking about code comments for example).

@Zabuzard Zabuzard dismissed I-Al-Istannen’s stale review October 18, 2021 14:15

All changes have been implemented.

@Zabuzard
Copy link
Member

Zabuzard commented Oct 19, 2021

NotNull found out that the MarkdownSanitizer.escape(...) does not correctly escape \\. It leaves it as \\ instead of making \\\\. We have to test whether the raw-command works with such input and whether its an actual issue in practice (thinking about code comments for example).

It was indeed a problem. I pushed code to fix it now.

* markdown escaper doesnt escape backslash \ correctly.
* introduced own escaper layer to correct it
* covered by unit tests
Spotless after comment
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@illuminator3 illuminator3 merged commit 58afeb7 into develop Oct 20, 2021
@Zabuzard Zabuzard deleted the feature/tag-system branch October 20, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new command Add a new command or group of commands to the bot priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate tag system
6 participants