Navigation Menu

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

Message#getLink method. #535

Merged
merged 2 commits into from May 2, 2020
Merged

Message#getLink method. #535

merged 2 commits into from May 2, 2020

Conversation

Joshix-1
Copy link
Member

@Joshix-1 Joshix-1 commented May 1, 2020

It is really helpful to have such a method. Some don't know how the links are composed, and for others it is helpful do just use the Javacord method instead of creating an own.

@Joshix-1 Joshix-1 changed the title Added Message#getLink function. Added Message#getLink method. May 1, 2020
@Joshix-1 Joshix-1 changed the title Added Message#getLink method. Message#getLink method. May 1, 2020
*
* @return The message link.
*/
default String getLink() {
Copy link
Member

Choose a reason for hiding this comment

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

Should this and the others probably return URL instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if that's better. I would think it is more convenient to leave it as String. I don't see any use case where it would be better to have it as an URL.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it is an URL, that could be reason enough.
But I'll let that decision for @Bastian, I just mentioned it to be thought about. :-)

Copy link
Member

Choose a reason for hiding this comment

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

I have to agree with Vampire. It is a url and thus should return a URL object.
This is also consistent with all the other methods of Javacord that return a url.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I will change that :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

How should I handle the MalformedURLException?

Copy link
Member

Choose a reason for hiding this comment

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

In other places we just return null and log a error, but this would be better:

try {
    URL url = new URL("");
} catch (MalformedURLException e) {
    throw new AssertionError("Message link is malformed", e);
}

Copy link
Member

Choose a reason for hiding this comment

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

I would have also said catch and transform to AssertionError.
As we construct the URL and know it is valid, it is a programming error if a MURLE is thrown, so translating it to an AssertionError is the way to go.

@Vampire
Copy link
Member

Vampire commented May 1, 2020

Also you should change the target branch of the PR to development

@Vampire
Copy link
Member

Vampire commented May 1, 2020

And you should execute check task, or at least look what our CI said, as you violated the defined checkstyle rules. Please fix up your commits, so that each one results in a green build.

@Joshix-1 Joshix-1 changed the base branch from master to development May 1, 2020 15:26
@Vampire
Copy link
Member

Vampire commented May 1, 2020

Please rebase your branch instead of merging in development and fix up each commit so that it has a green checkmark at https://github.com/Javacord/Javacord/pull/535/commits meaning each commit individually results in a successful build.
You might even consider making one commit of them, the changes are pretty atomic alltogether I'd say. :-)

@Bastian
Copy link
Member

Bastian commented May 2, 2020

Besides the method return type, this looks good to me.
Thanks! :)

@Bastian Bastian added the low priority An issue or pull request with a low priority label May 2, 2020
@Bastian Bastian added this to the Next Version milestone May 2, 2020
@Bastian Bastian merged commit 04946e7 into Javacord:development May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority An issue or pull request with a low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants