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.

Sign up for GitHub

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

Jump to bottom

Invalid token exception #2025

Merged
merged 15 commits into from
Aug 30, 2022
Merged

Invalid token exception #2025

merged 15 commits into from
Aug 30, 2022

Conversation

java-coding-prodigy
Copy link
Contributor

@java-coding-prodigy java-coding-prodigy commented Feb 9, 2022 •
edited

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

Replaces LoginException with InvalidTokenException

TODO:

  • Make the exception class
  • Add documentation
  • Replace usages

Closes issue: https://github.com/DV8FromTheWorld/JDA/projects/5#card-75890666

@java-coding-prodigy
Copy link
Contributor Author

Ah great, what did I mess up now...

@java-coding-prodigy
Copy link
Contributor Author

Oh thank god I fixed that(For those visiting this later I basically accidently added 7 commits from a different branch, which I removed now).

@Tais993
Copy link
Contributor

Tais993 commented Feb 10, 2022

I'd make this a draft PR unless it's finished, if it's finished you should check the TO-DO's you added.

Tais993
Tais993 reviewed Feb 10, 2022
View reviewed changes
src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/internal/JDAImpl.java
@@ -284,7 +280,7 @@ public int login(String gatewayUrl, ShardInfo shardInfo, Compression compression
String token = authConfig.getToken();
setStatus(Status.LOGGING_IN);
if (token == null || token.isEmpty())
throw new LoginException("Provided token was null or empty!");
throw new InvalidTokenException("Provided token was null or empty!");
Copy link
Contributor

Choose a reason for hiding this comment

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

@DV8FromTheWorld doesn't it make more sense to throw an IllegalArgument here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo no, we should use this exception.

Copy link
Member

Choose a reason for hiding this comment

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

This check should have been done before hand in JDABuilder, not here.
We should move this token check to the JDABuilder and use Checks.notEmpty to ensure it is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will this be done in this PR or should we make a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

We can push this to another PR

src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
src/main/java/net/dv8tion/jda/internal/utils/InvalidTokenException.java Outdated Show resolved Hide resolved
@Sanduhr32
Copy link
Contributor

Sanduhr32 commented Feb 27, 2022 •
edited

Whats your idea behind this? LoginException perfectly fits here imo.
PS: PR description missing, todo

@Tais993
Copy link
Contributor

Tais993 commented Feb 27, 2022 •
edited

There have been discussions about this in lib-dev, check there for the exact motivation.

One of them was because this is unchecked, instead of the annoying checked exception we're used to having

@MinnDevelopment MinnDevelopment added the type: breaking contains a backwards incompatible change label Apr 15, 2022
@java-coding-prodigy
Copy link
Contributor Author

This PR seems to be completed.
Will this feature be implemented?

DManstrator
DManstrator reviewed Jul 7, 2022
View reviewed changes
src/main/java/net/dv8tion/jda/api/sharding/ShardManager.java Outdated
Comment on lines 1166 to 1167
void login() throws LoginException;
void login() ;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove the space between the parenthesis and the semicolon.

Edit: I just saw that there is generally a space after each method which had a throws LoginException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I need to remove all of them right?

@java-coding-prodigy
Copy link
Contributor Author

Should I keep the file in internal.utils or should I move it to api.exceptions since that's where all the exceptions should be.

@java-coding-prodigy java-coding-prodigy force-pushed the invalid_token_exception branch 2 times, most recently from 11b4240 to e008e01 Compare July 17, 2022 03:36
@java-coding-prodigy
Copy link
Contributor Author

I have applied the changes suggested in the code review.

@java-coding-prodigy java-coding-prodigy force-pushed the invalid_token_exception branch 3 times, most recently from 6a8bda0 to 9ae0f6f Compare July 26, 2022 13:55
@java-coding-prodigy
Copy link
Contributor Author

I have made the necessary changes quite some time ago yet this PR is not receiving any recognition.

@DV8FromTheWorld
Copy link
Member

Also, https://github.com/DV8FromTheWorld/JDA/pull/2025/files#r917301036 still has not been addressed.
I guess we could technically do that in a separate PR

This PR hasn't had much attention put on it because it is extremely low priority. We are trying to get the massive breaking changes done so we can exit Alpha. This will be included in the alpha, but as one of the last things to get us over the finish line.

@java-coding-prodigy @DV8FromTheWorld
Update src/main/java/net/dv8tion/jda/api/exceptions/InvalidTokenExcep…
d1eaf63 
…tion.java

Co-authored-by: Austin Keener <keeneraustin@yahoo.com>
@DV8FromTheWorld DV8FromTheWorld merged commit 31b1bfb into discord-jda:master Aug 30, 2022
@renovate renovate bot mentioned this pull request Aug 11, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DManstrator DManstrator DManstrator left review comments

@Chew Chew Chew left review comments

@Tais993 Tais993 Tais993 left review comments

@DV8FromTheWorld DV8FromTheWorld DV8FromTheWorld approved these changes

Assignees
No one assigned
Labels
type: breaking contains a backwards incompatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@java-coding-prodigy @Tais993 @Sanduhr32 @DV8FromTheWorld @DManstrator @Chew @MinnDevelopment