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

ChatGPT response Dismiss button #901

Merged
merged 3 commits into from
Oct 22, 2023

Conversation

SquidXTV
Copy link
Member

About

closes #897

Adds the dismiss button to the chatgpt response:
image

@SquidXTV SquidXTV requested review from a team as code owners September 24, 2023 12:36
@SquidXTV
Copy link
Member Author

Two questions:

  1. Is the concurrent arraylist needed in HelpSystemHelper or would a normal one work as well?
  2. The event.getMember seems to be @Nullable, should I care? because the filesharing feature didnt care as well

Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

  1. Is the concurrent arraylist needed in HelpSystemHelper or would a normal one work as well?

Yes, the lambda can be invoked from multiple different threads and also concurrently.

  1. The event.getMember seems to be @nullable, should I care? because the filesharing feature didnt care as well

What does the javadoc (or source code) say? When will it be null? Once you checked that, is the situation a situation u want to support, or do you consider the situation a bug/impossible?

@SquidXTV
Copy link
Member Author

It says this:

This is null if the interaction is not from a guild.

What is a guild in this context?

@Zabuzard
Copy link
Member

Zabuzard commented Sep 25, 2023

It says this:

This is null if the interaction is not from a guild.

What is a guild in this context?

Guild = Server

Think about buttons in a DM only conversation. Then you have no server-context.

Member = User + in a Server

There is also event.getUser() to get it without the server-context. Which is not nullable.

@ankitsmt211
Copy link
Member

ankitsmt211 commented Sep 25, 2023

What is a guild in this context?

Guild refers to "server" from what I know.

@SquidXTV
Copy link
Member Author

Though I need a Member to check the roles.
Can I ignore it because it will not be null

@Zabuzard
Copy link
Member

Zabuzard commented Sep 25, 2023

Though I need a Member to check the roles. Can I ignore it because it will not be null

You are wording this wrong. It is not about ignoring it. It is about explicitly demanding that it must not be null.

I.e. if it is null, you consider it as fatal error / programmer bug. And what should happen for bugs? Crash! So implicitly invoking a NPE is a deliberate choice and the correct design.

Although unecessary, you can make this even more explicit by saying Objects.requireNonNull(foo).bar() instead of foo.bar().

@SquidXTV
Copy link
Member Author

Though I need a Member to check the roles. Can I ignore it because it will not be null

You are wording this wrong. It is not about ignoring it. It is about explicitly demanding that it must not be null.

I.e. if it is null, you consider it as fatal error / programmer bug. And what should happen for bugs? Crash! So implicitly invoking a NPE is a deliberate choice and the correct design.

Although unecessary, you can make this even more explicit by saying Objects.requireNonNull(foo).bar() instead of foo.bar().

yeah sorry I was in school lol
didnt have much time thinking about how to write it, thanks

@SquidXTV SquidXTV force-pushed the feature/chatgpt-answer-dismiss-button branch from af21cfc to 8437399 Compare September 27, 2023 19:14
@SquidXTV SquidXTV force-pushed the feature/chatgpt-answer-dismiss-button branch from d1c34c0 to f404d3b Compare October 22, 2023 15:00
@sonarcloud
Copy link

sonarcloud bot commented Oct 22, 2023

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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Zabuzard
Copy link
Member

has been approved since 2 weeks. will be merged now.

@Zabuzard Zabuzard merged commit 9ea9cbd into develop Oct 22, 2023
11 checks passed
@Zabuzard Zabuzard deleted the feature/chatgpt-answer-dismiss-button branch October 22, 2023 17:47
@Zabuzard Zabuzard mentioned this pull request Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dismiss Button for ChatGPT answer
4 participants