-
Notifications
You must be signed in to change notification settings - Fork 5
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
MojangAPI and textures #102
base: master
Are you sure you want to change the base?
Conversation
bukkit/src/main/java/com/dumbdogdiner/stickyapi/bukkit/command/BukkitCommandBuilder.java
Outdated
Show resolved
Hide resolved
*/ | ||
package com.dumbdogdiner.stickyapi.webapi.mcapi; | ||
|
||
class Ping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one class for just two vars? please move these to wherever they are used, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very much agree with koda here - what's this used for? Likely for ping status yes, but imo this is slightly overkill.
*/ | ||
package com.dumbdogdiner.stickyapi.webapi.namemc; | ||
|
||
class Profile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove empty classes
*/ | ||
package com.dumbdogdiner.stickyapi.webapi.namemc; | ||
|
||
class Server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove empty classes
Re review of irrelevant bits: currently working on pulling these out |
05c3e57
to
8f9a805
Compare
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
==========================================
+ Coverage 34.72% 34.82% +0.09%
==========================================
Files 63 83 +20
Lines 2534 3024 +490
Branches 286 332 +46
==========================================
+ Hits 880 1053 +173
- Misses 1591 1890 +299
- Partials 63 81 +18
Continue to review full report at Codecov.
|
TODO: add caching support to MojangAPI so it can cache things that are just used, maybe even just a 60 second cache, but it helps if a few things are needed at once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay - some formatting complaints. Lots of empty classes/seemingly redundant code - are these needed?
@@ -2,6 +2,10 @@ plugins { | |||
id "io.freefair.lombok" version "5.3.0" apply false | |||
id "com.github.hierynomus.license" version "0.15.0" apply false | |||
|
|||
// root project - intellij | |||
|
|||
id 'idea' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is up with all this spacing lul
build.gradle
Outdated
@@ -10,6 +14,8 @@ allprojects { | |||
group = "com.dumbdogdiner" | |||
version = "2.2.0" | |||
|
|||
apply plugin: "idea" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javadoc.options.encoding = "UTF-8" | ||
tasks.withType(Javadoc) { | ||
options.addBooleanOption("quiet", true) | ||
options.encoding = "UTF-8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be specified if it's in allprojects {}
, no?
@@ -28,6 +28,7 @@ dependencies { | |||
These is also an accompanying test at (test src) com.dumbdogdiner.stickyapi.StickyAPITest | |||
*/ | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Do not call a method annotated with this, it will do bad things | ||
*/ | ||
@Documented | ||
public @interface DoNotCall { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this existtttt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check where it's used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point still stands? This isn't really something we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its needed because if any of the methods are called, it will crash, so its for documentation, and can be used with google's errorprone (or that can be used) to make sure they are not called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but please don't add things unrelated to a PR without asking the rest of the team - for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also like to add that if you dont want a specific method to be called, please use the correct modifier for your use case instead of creating annotations that wont prevent the user from executing this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be able to do stuff via refactor though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloned from previous PR:
Would prefer it if we can write things in a way where methods that are strictly internal can't be called externally.
So yes, would be nice if you can refactor this in a way that doesn't cause this issue.
*/ | ||
|
||
|
||
class AshconResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal classes in here are lacking javadoc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do internal data classes need javadoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we still need to read this code when we're adding to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do internal data classes need javadoc?
We still need to understand and read the code.
|
||
|
||
// Double check the occurances of this and whatnot, try to fix, etc. | ||
public class MojangAPI { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of class would be nice :3 + javadoc for methods
private static final @NotNull OkHttpClient CLIENT = HttpUtil.getDefaultClientInstance(); | ||
|
||
|
||
protected static final @NotNull HttpUrl COMBINED_API_URL = new HttpUrl.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using strings be easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean using .parse? I'm changing to that. HttpUrl is useful for setting flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you end up changing to .parse
?
import java.util.UUID; | ||
import java.util.stream.Collectors; | ||
|
||
public class NameMC { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments for MojangAPI
import java.util.List; | ||
import java.util.UUID; | ||
|
||
class ScrapedProfile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc :3
Empty classes will either be filled to spec or removed prior to merging. Have to check for duplicate code, I did my best but haven't checked changelog |
I have no idea what comments from the review I made earlier are valid here, but worth checking some of those. If I get some time tomorrow I will review this too. |
@SkyezerFox This is the PR that needs review, the other one is for reference only, and is a very old version of the code |
Am knows, hence my comment yesterday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things still valid from the previous PR:
- Formatting, but we can fix that with spotless so don't worry.
- Pls see comments for a couple of specific things - there are still some unresolved comments above!
- thx for working so hard <3
- friendly neighborhood floofball
|
||
import java.io.File; | ||
|
||
public class StickyAPIPlugin extends JavaPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloned from the previous review:
Did we ever agree on how we would implement this? Thought for quite a long time it makes much more sense to just pass plugin-specific data about. Do correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember us agreeing, and I also don't remember an easy way to do testing on plugin type functions without making a mocked plugin. please correct me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, can you refactor this out into a separate branch and then we can discuss how it would be implemented from there? Would be highly appreciated <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its needed for testing heads, I can refactor but it's a pain due to testing. @Prouser123 thoughts?
textures/build.gradle
Outdated
|
||
implementation("com.squareup.okhttp3:okhttp") | ||
implementation("com.squareup.okhttp3:logging-interceptor") | ||
implementation 'commons-validator:commons-validator:1.7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommonsValidator also allows us to remove a bunch of the luhn/shortid code we have; I can write a URL regex if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)
here's one :3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be very appreciated if you can stick with Preconditions since we can technically use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a special one, and it should be in there, not sure if pushed. Included domain checking
import java.time.Instant; | ||
|
||
@UtilityClass | ||
public class BlacklistChecks { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JavaDoc pls <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the entire class :3
|
||
/** | ||
* Provides the UUID associated with a given username at a given time. | ||
* FIXME: This is currently broken, with an <a href="https://bugs.mojang.com/browse/WEB-3367>open bug report at Mojang</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like JavaDoc might complain about this.
* FIXME: This is currently broken, with an <a href="https://bugs.mojang.com/browse/WEB-3367>open bug report at Mojang</a> | |
* FIXME: This is currently broken, with an <a href="https://bugs.mojang.com/browse/WEB-3367">open bug report at Mojang</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks broken on my end still. Not included in commit perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just marking them off as I go so I'm not a dumbwoof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I've been asked to make less commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who told you that? Many commits are fine as long as you label them properly :3
* Do not call a method annotated with this, it will do bad things | ||
*/ | ||
@Documented | ||
public @interface DoNotCall { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloned from previous PR:
Would prefer it if we can write things in a way where methods that are strictly internal can't be called externally.
So yes, would be nice if you can refactor this in a way that doesn't cause this issue.
@@ -1,6 +1,6 @@ | |||
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.3-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From previous PR:
For future reference: please don't change Gradle versions without asking one of us!
@DoNotCall | ||
@Override | ||
@Deprecated | ||
public @NotNull SkullBuilder texture(@NotNull String str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments on DoNotCall
.
public static String getName(int headId) { | ||
if(usePlugin) | ||
return getNamePlugin(headId); | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary else statement - return breaks out of the method anyway.
In regards to your comment here, #56 (comment), that's not the code style we're using. Here's Google's styleguide - must draw attention to the section on immutable variables:
Given that this is a |
Formatting complaints can be ignored for now - will be fixed when we get spotless working. |
@SkyezerFox so, i cant use C style for statics? |
It's not specified in the code style we're planning to use, so I wouldn't do it here while we wait to implement it, no. |
Can we discuss adding it to our styleguide? It's nice to identify statics |
We can add it to the next meeting, but pls stick with no C statics until we decide. |
My god the build process... Anyway, building fails and there are current conflicts. Please resolve these and await the re-requested reviews. Note to reviewers: When the above requested changes are completed, you will have 7 days to review before this gets merged. |
Only thing I'd like to add (not going to review in detail), but knowing you, there definitely are bits in this PR that are entirely unrelated to MojangAPI and a thing to get heads. If not done already, I'd ask you to remove them from this PR and open a new one with a proper description of what you want changed (although please don't do this now <3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good, still a bunch of comments left over from my March review + added some more - pls fix :3
meta = (SkullMeta) head.getItemMeta(); | ||
Preconditions.checkNotNull(meta, "Player head must have metadata attached"); | ||
// We check this almost immediately after, so it's OK if it's null temporarily | ||
//noinspection ConstantConditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u b careful not to leave editor specific comments around?
/** | ||
* @param playerId the UUID of the player whose head should be generated | ||
*/ | ||
public PlayerHeadBuilder(@NotNull UUID playerId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
@Getter | ||
@NotNull | ||
@VisibleForTesting | ||
private String category = "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
|
||
profile.setProperty(new ProfileProperty("textures", texture)); | ||
meta.setPlayerProfile(profile); | ||
@NotNull ItemStack head = new ItemStack(Material.PLAYER_HEAD, quantity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for @NotNull
annotations in code - unless you want to add them, kotlin can't do anything useful with them since they're not externally visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its more for reading and notes, and also because when you auto add them it adds them everywhere
@@ -0,0 +1,75 @@ | |||
plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a need for a consistent quoting/brackets commit here. Are we doing implementation thing
or implementation(thing)
, ""
or ''
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, if @Prouser123 agrees, it will be kotlin style syntax. When you cut and paste from different things, you get different syntax.
private static final HeadDatabaseAPI mcheads; | ||
|
||
static { | ||
//noinspection ConstantConditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
//noinspection ConstantConditions | ||
if(ServerVersion.isBukkit() && Bukkit.getServer() != null) { | ||
if (Bukkit.getPluginManager().getPlugin("HeadDatabase") == null) { | ||
Bukkit.getLogger().severe("HeadDatabase Plugin was not found!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely some logic simplifying possible here - you set them null if none of these conditions match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have the heads database plugin availible (either because its not installed or because we arent on bukkit) then it goes to fallback
|
||
for (int i = 0; i < TEST_REPS; i++) { | ||
HeadThing thing; | ||
//noinspection StatementWithEmptyBody |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
this(DEFAULT_DOMAIN_REGEX); | ||
} | ||
|
||
private UrlValidator(Pattern domain){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a private method, but for future devs can you comment on what it does?
import java.time.Instant; | ||
|
||
@UtilityClass | ||
public class BlacklistChecks { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the entire class :3
An additional comment - why is the config package completely empty now? |
Might b worth cherry-picking stuff since this PR is chonky af. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note on MojangAPI
private static final @NotNull OkHttpClient CLIENT = HttpUtil.getDefaultClientInstance(); | ||
|
||
|
||
protected static final @NotNull HttpUrl COMBINED_API_URL = new HttpUrl.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you end up changing to .parse
?
Changing what to .parse? |
dd42e0c
to
bc947a5
Compare
It would be super nice to have a way to interact with Mojang's web api; and it would also be nice ot have some utility methods for textures
BOTH are in one pr because they interact with each other a bunch, especially in testing