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

[REFERENCE ONLY DO NOT REVIEW!!!] Implement Mojang WebAPI, common user root class, and playerheads #56

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

aakatz3
Copy link
Collaborator

@aakatz3 aakatz3 commented Dec 20, 2020

Closes #51

@aakatz3
Copy link
Collaborator Author

aakatz3 commented Dec 20, 2020

Still todo: figure out tests

@aakatz3 aakatz3 added this to the next (v2.0) milestone Dec 20, 2020
@aakatz3 aakatz3 added feature New feature or request non-breaking labels Dec 20, 2020
@aakatz3 aakatz3 self-assigned this Dec 20, 2020
@aakatz3 aakatz3 marked this pull request as ready for review December 25, 2020 19:39
@codecov
Copy link

codecov bot commented Dec 25, 2020

Codecov Report

Merging #56 (7f2cab7) into master (a6e96d8) will increase coverage by 6.22%.
The diff coverage is 31.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #56      +/-   ##
============================================
+ Coverage     20.39%   26.61%   +6.22%     
- Complexity      111      196      +85     
============================================
  Files            63       76      +13     
  Lines          2859     3179     +320     
  Branches        381      409      +28     
============================================
+ Hits            583      846     +263     
- Misses         2261     2278      +17     
- Partials         15       55      +40     
Impacted Files Coverage Δ Complexity Δ
...ogdiner/stickyapi/bukkit/command/AsyncCommand.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...stickyapi/bukkit/command/BukkitCommandBuilder.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...umbdogdiner/stickyapi/bukkit/command/ExitCode.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...gdiner/stickyapi/bukkit/command/PluginCommand.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...iner/stickyapi/bukkit/generator/VoidGenerator.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...mbdogdiner/stickyapi/bukkit/gui/ClickableSlot.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ava/com/dumbdogdiner/stickyapi/bukkit/gui/GUI.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...diner/stickyapi/bukkit/item/PlayerHeadBuilder.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...iner/stickyapi/bukkit/particle/ParticleSystem.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...diner/stickyapi/bukkit/particle/shapes/Circle.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6e96d8...7f2cab7. Read the comment docs.


import java.util.UUID;

public class StickyUserBukkit extends StickyUser {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we decided not to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We definitely decided not to do this.


import java.util.UUID;

public class StickyUserBungee extends StickyUser {
Copy link
Member

Choose a reason for hiding this comment

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

see above

Copy link
Member

Choose a reason for hiding this comment

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

See above again


import java.util.UUID;

public class StickyUser implements Cacheable {
Copy link
Member

Choose a reason for hiding this comment

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

see above

@@ -0,0 +1,173 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Oh my... Is this used for heads?

Move to resources please.

Copy link
Member

Choose a reason for hiding this comment

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

If this is moved to resources please remove this class.

* Copyright (c) 2020 DumbDogDiner <dumbdogdiner.com>. All rights reserved.
* Licensed under the MIT license, see LICENSE for more information...
*/
package com.dumbdogdiner.stickyapi.common.webapis;
Copy link
Member

Choose a reason for hiding this comment

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

com.dumbdogdiner.stickyapi.common.webapis to com.dumbdogdiner.stickyapi.common.mojang

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thought was other apis, such as optifine capes or labymod capes, or any other web apis, can be put in here. I will make it com.dumbdogdiner.stickyapi.common.webapi.mojang unless thats too long

Copy link
Member

Choose a reason for hiding this comment

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

com.dumbdogdiner.stickyapi.common.web.mojang

Copy link
Member

Choose a reason for hiding this comment

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

bump

Comment on lines 12 to 14



Copy link
Member

Choose a reason for hiding this comment

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

Don't add empty files please.


import lombok.Getter;

public enum DefaultSkins {
Copy link
Member

Choose a reason for hiding this comment

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

Move to resources please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What format should these be in in resources? SQLite DB??

Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON.

import com.dumbdogdiner.stickyapi.common.util.StringUtil;
import lombok.Getter;

public enum MobHead {
Copy link
Member

Choose a reason for hiding this comment

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

Move to resources.

import java.time.Instant;
import java.util.*;

public class MojangUser extends StickyUser {
Copy link
Member

@ZachyFoxx ZachyFoxx Dec 25, 2020

Choose a reason for hiding this comment

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

Instead of using StickyUser, just make this its own object with the basic user information we'd need, such as username, uuid, skin textures, etc. Pretty much an object for the response from Mojang's API.

Copy link
Member

Choose a reason for hiding this comment

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

bump


import java.util.UUID;

public class MobHeadGenerator {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MobHeadBuilder?

* API URLs
*/

public enum APIStatus{
Copy link
Collaborator

Choose a reason for hiding this comment

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

inb4 vlad complains about the lack of a space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops

Choose a reason for hiding this comment

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

inb4 vlad complains about the lack of a space.

you know me too well <3


public Map<String, List<Instant>> getUsernameHistory(){
HashMap<String, List<Instant>> usernameHistory = new HashMap<>();
for (AshconResponse.Username u : apiResponse.username_history) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider calling this variable previous or prev?

if(usernameHistory.containsKey(u.username) && u.changed_at != null){
usernameHistory.get(u.username).add(Instant.parse(u.changed_at));
} else {
ArrayList<Instant> hist = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ArrayList<Instant> hist = new ArrayList<>();
ArrayList<Instant> history = new ArrayList<>();

/**
* This package contains code that is used for obtaining information from Mojang's web API or cached versions. Other APIs may be supported in the future.
*/
package com.dumbdogdiner.stickyapi.common.webapis;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
package com.dumbdogdiner.stickyapi.common.webapis;
package com.dumbdogdiner.stickyapi.common.webapis;

void getUsername() {
assertEquals(new CachedMojangAPI(UUID.fromString("9b6d27b3-f53b-49d1-b8c4-fa807f7575e9")).getUsername(), "Rodwuff");
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

@aakatz3 aakatz3 force-pushed the feat/heads-mojangapi-users branch from 5cb2f06 to 9722bf7 Compare December 26, 2020 07:25
@aakatz3 aakatz3 force-pushed the feat/heads-mojangapi-users branch from 9722bf7 to b014f6b Compare December 26, 2020 07:29
@aakatz3 aakatz3 force-pushed the feat/heads-mojangapi-users branch from 17dfcfc to 329515f Compare December 26, 2020 07:38
Copy link
Collaborator

@kaylendog kaylendog left a comment

Choose a reason for hiding this comment

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

Dun worry about the formatting comments - I'm just nitpicking. Those will be fixed by themselves once we merge in #58.

implementation 'org.jetbrains:annotations:20.1.0'
implementation 'com.google.code.gson:gson:2.8.6'
implementation 'io.github.classgraph:classgraph:4.8.92'
implementation 'com.github.seancfoley:ipaddress:5.3.3'
implementation 'com.squareup.okhttp3:okhttp:4.9.0'
implementation 'commons-validator:commons-validator:1.7'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use Guava's preconditions rather than shading in deps to StickyAPI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please explain

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the JavaDocs for preconditions. This is included in both bukkit and bungee so will work at runtime. No need to shade in extra dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you're intending to use this for a completely different reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Validator is used primarily to check url formatting for validity, and determine if a URL is validly formatted (and I think it also checks if it's resolvable). There are a couple other ways I plan to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can also theoretically call the minimize() function to reduce jar size

Copy link
Collaborator

Choose a reason for hiding this comment

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

Validator is used primarily to check url formatting for validity, and determine if a URL is validly formatted (and I think it also checks if it's resolvable). There are a couple other ways I plan to use it.

Google preconditions does thissssss - just include a URL regex.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also theoretically call the minimize() function to reduce jar size

This has side effects, we've already tried it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even need to validate the URL ourselves? OkHttp should throw an exception when unable to parse a URL, and if we don't want to use OkHttp, a regular expression will do for URL validity

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also probably test URL validity by attempting to construct a URL object using the string and seeing if a MalformedURLException is thrown

@@ -0,0 +1 @@
systemProp.file.encoding=utf-8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
systemProp.file.encoding=utf-8
systemProp.file.encoding=utf-8

TestsCommon.superficialEnumCodeCoverage(DefaultSkins.class);
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

void testEnum(){
TestsCommon.superficialEnumCodeCoverage(MiniBlock.class);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

public void testHead(){
TestsCommon.superficialEnumCodeCoverage(MobHead.class);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

@aakatz3
Copy link
Collaborator Author

aakatz3 commented Dec 27, 2020 via email

…sgraph-classgraph-4.8.98' into feat/heads-mojangapi-users
@aakatz3 aakatz3 requested a review from ZachyFoxx January 9, 2021 03:53
build.gradle Outdated
Comment on lines 73 to 75
testImplementation("org.mockito:mockito-core:3.7.0")
testImplementation 'com.github.seeseemelk:MockBukkit-v1.16:0.5.0'
testImplementation 'it.unimi.dsi:fastutil:8.4.4'
Copy link
Member

Choose a reason for hiding this comment

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

Can we start adding comments for depends so we know why they're added so I don't have to ask like this later...

test {
useJUnitPlatform()
testLogging {
events "passed", "skipped", "failed"
// Show System.out for code ran by tests
showStandardStreams = true
}
//ignoreFailures = true
Copy link
Member

Choose a reason for hiding this comment

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

remove this line if it's commented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its there for testing. I will try to fold into a new task

Comment on lines 35 to 37
public static InputStream getResourceAsStream(String resourceName){
return StickyAPI.class.getResourceAsStream(resourceName);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why? Also, where are the annotations?

import java.util.Objects;
import java.util.UUID;

public class PlayerHeadBuilder extends SkullBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

JAVADOC!!!!

Click me


}

public @NotNull SkullBuilder filter(@NotNull String group) {
Copy link
Member

Choose a reason for hiding this comment

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

@@ -64,7 +64,7 @@ public static TextComponent convertURLs(@NotNull String text) {

TextComponent urlComponent = new TextComponent(url.getShortened() + " ");
urlComponent.setClickEvent(new ClickEvent(ClickEvent.Action.OPEN_URL, url.getFullPath()));
urlComponent.setHoverEvent(new HoverEvent(HoverEvent.Action.SHOW_TEXT, new Text("§7Click to open URL"), new Text("\n§8" + url.getFullPath())));
urlComponent.setHoverEvent(new HoverEvent(HoverEvent.Action.SHOW_TEXT, new Text(ChatColor.COLOR_CHAR + "" + ChatColor.GRAY + "Click to open URL"), new Text("\n§8" + url.getFullPath())));
Copy link
Member

Choose a reason for hiding this comment

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

ChatColor.COLOR_CHAR + "" is unnecessary if you have ChatColor.GRAY ahead of it.

* Copyright (c) 2020 DumbDogDiner <dumbdogdiner.com>. All rights reserved.
* Licensed under the MIT license, see LICENSE for more information...
*/
package com.dumbdogdiner.stickyapi.common.webapis;
Copy link
Member

Choose a reason for hiding this comment

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

bump


//TODO: Better error handeling in case of 404

public class MojangAPI {
Copy link
Member

Choose a reason for hiding this comment

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

bump

import java.time.Instant;
import java.util.*;

public class MojangUser extends StickyUser {
Copy link
Member

Choose a reason for hiding this comment

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

bump

import java.time.Instant;
import java.util.*;

public class MojangUser extends StickyUser {
Copy link
Member

Choose a reason for hiding this comment

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

and bump

@ZachyFoxx
Copy link
Member

Note i did that review on no sleep, i'll do another one later.

@aakatz3
Copy link
Collaborator Author

aakatz3 commented Jan 9, 2021

Resolves #62

@ZachyFoxx ZachyFoxx linked an issue Jan 9, 2021 that may be closed by this pull request
tasks.delombok.shouldRunAfter(sources)
tasks.publish.dependsOn build
tasks.build.shouldRunAfter(clean)
tasks.javadoc.shouldRunAfter(clean)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little hesitant to have javadoc and build run after clean...

@NotZachery thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this could become an issue so I'm fine by it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are set to run after as should, not forced. This prevents (if you did, say, ./gradlew build clean javadoc it from building then deleting the build.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay - thanks!

Copy link
Member

@ZachyFoxx ZachyFoxx left a comment

Choose a reason for hiding this comment

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

TL;DR

No wildcard imports, new line at end of files, and formatting.

* Do not call a method annotated with this, it will do bad things
*/
@Documented
public @interface DoNotCall {
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this exactly? If a method shouldn't be called why does it exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer it if we can write things in a way where methods that are strictly internal can't be called externally.

import org.bukkit.command.CommandMap;
import org.bukkit.command.CommandSender;
import org.bukkit.command.TabCompleter;
import org.bukkit.command.*;
Copy link
Member

Choose a reason for hiding this comment

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

No wild card imports.

public @NotNull SkullBuilder category(@NotNull String str) {
throw new UnsupportedOperationException();
}
}
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
}
}

category(qn[0]);
return head(qn[1]);
}
}
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
}
}

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

no wildcard imports

import lombok.Getter;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.sql.Timestamp;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

no wildcard imports

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.io.*;
Copy link
Member

Choose a reason for hiding this comment

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

no wildcard imports

import java.util.HashMap;
import java.util.Map;
import java.util.UUID;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

no wildcard imports

import java.io.InputStreamReader;
import java.lang.reflect.Type;
import java.net.URL;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

no wildcard imports

Comment on lines +116 to +129
if (splits[0].equals("*")) {
@Nullable String texture = null;
for (@NotNull String cat : getCategories()) {
try {
texture = getTexture(cat, qualifiedName);
} catch (NoSuchElementException e) {
continue;
}
return texture;
}
return texture;
} else {
return getTexture(splits[0], splits[1]);
}
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
if (splits[0].equals("*")) {
@Nullable String texture = null;
for (@NotNull String cat : getCategories()) {
try {
texture = getTexture(cat, qualifiedName);
} catch (NoSuchElementException e) {
continue;
}
return texture;
}
return texture;
} else {
return getTexture(splits[0], splits[1]);
}
if (!splits[0].equals("*"))
return getTexture(splits[0], splits[1]);
@Nullable String texture = null;
for (@NotNull String cat : getCategories()) {
try {
texture = getTexture(cat, qualifiedName);
} catch (NoSuchElementException e) {
continue;
}
return texture;
}
return texture;

@ZachyFoxx ZachyFoxx marked this pull request as draft March 23, 2021 17:47
Copy link
Collaborator

@kaylendog kaylendog left a comment

Choose a reason for hiding this comment

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

A couple of things:

  1. Formatting, but we can fix that with spotless so don't worry.
  2. No need to worry about @Nullable and @NotNull annotations in actual method bodies - they're only needed on method signatures so the KLS can work out what is going on.
  3. Need to discuss splitting this into 3 PRs, one for the Web API, the user class, and player heads.
  4. Pls see comments for a couple of specific things.
  5. thx for working so hard <3
  • friendly neighborhood floofball

@@ -1,6 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-6.7-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-6.8.2-bin.zip
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future reference: please don't change Gradle versions without asking one of us!

* Do not call a method annotated with this, it will do bad things
*/
@Documented
public @interface DoNotCall {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer it if we can write things in a way where methods that are strictly internal can't be called externally.

@@ -43,7 +43,7 @@
* @param commandName The name of the command the user will execute
* @param owner The plugin that owns this command.
*/
public AsyncCommand(String commandName, Plugin owner) {
public AsyncCommand(@NotNull String commandName, Plugin owner) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future reference: please keep changes unrelated to the branch somewhere else!

if (args == null)
throw new NullPointerException("arguments to tabComplete cannot be null");

List<String> completions = null;
@org.jetbrains.annotations.Nullable List<String> completions = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe these annotations are for the KLS to help infer things properly - don't think we need them in internal Java code.

@@ -181,16 +181,16 @@ public void setTabCompleter(TabCompleter completer) {
*/
@Override
public java.util.@NotNull List<String> tabComplete(@NotNull CommandSender sender, @NotNull String alias,
String[] args) throws CommandException, IllegalArgumentException {
String @org.jetbrains.annotations.Nullable [] args) throws CommandException, IllegalArgumentException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wacky import? If this can be imported as normal, would be appreciated.

static final OkHttpClient httpClient = new OkHttpClient();
private static final Gson GSON = new Gson();
private static final Yaml YAML = new Yaml();
private static Map<String, Map<String, String>> TextureMap = generateTextureMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skeeball really doesn't like C-style variable names - do you use this to identify static vars?

Suggested change
private static Map<String, Map<String, String>> TextureMap = generateTextureMap();
private static Map<String, Map<String, String>> textureMap = generateTextureMap();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Global variables are in C-style (title case), caml case is local, all caps is constant - CS106a code style

Also trying to avoid this: https://youtu.be/GPWah4wbwYs?t=1264


import java.net.URL;

public class HttpException extends RuntimeException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Believe I've commented on this before - why the need for two layers of wrapping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's one layer of wrapping in most cases, but its using inheritance to subclass different types of exception so you can do different behaviors


//TODO: Better error handeling in case of 404

public class OldAPI {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this for?

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wildcard import!


import static org.junit.jupiter.api.Assertions.*;

class MojangAPITest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unit test for this would be lovely <3

@aakatz3 aakatz3 changed the title Implement Mojang WebAPI, common user root class, and playerheads [REFERENCE ONLY DO NOT REVIEW!!!] Implement Mojang WebAPI, common user root class, and playerheads Apr 20, 2021
@SolteraGG SolteraGG locked and limited conversation to collaborators Apr 20, 2021
@kaylendog kaylendog removed the request for review from spazzylemons April 20, 2021 15:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Player Head Generation ‘javadoc’ task fails on master
7 participants