Page MenuHomePhabricator

Added the User & LoggedUser classes
ClosedPublic

Authored by CLOVIS on Jul 20 2018, 4:29 PM.

Details

Summary
  • Better handling of the Accept headers etc & better exceptions
  • Added the DataType enum
  • Added unit test for GET /users/ via token
  • Made Request fluent for more control
  • Better fluent code!
  • Fixed missing trailing slash in test
  • Modifications
  • Method to clear the cache
  • Cleaning & Javadoc
  • Cleaning & Javadoc
  • Throw exception when querying the ID if not set
  • Better Javadoc + details about threading
  • Better users
  • Added the LoggedUser class
  • Unit tests for the User class
  • Added missing override annotations as typos
Test Plan

Unit tests

Diff Detail

Repository
rLIBWFJVA WildFyre Java Libary
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

CLOVIS added inline comments.
.arclint
22

This is because 80 chars is painful in an IDE that can take the full width of the screen

WyldBot added inline comments.
cache/src/main/java/net/wildfyre/descriptors/Descriptor.java
57

Add comments about why we are suppressing warnings and why a working fix is not possible

67

Read above

cache/src/main/java/net/wildfyre/descriptors/Internal.java
40

Explain more, read above

63

Why is the time being used here?

cache/src/main/java/net/wildfyre/descriptors/LoggedUser.java
50

Explain

63

Explain

76

Explain

100

Explain

cache/src/main/java/net/wildfyre/descriptors/User.java
206

You more than likely shouldn't override built in functions, you may need it in the future

218

Read above, make seperate functions

230

Read above, prefix with get?

cache/src/test/java/net/wildfyre/descriptors/UserTest.java
54

Explain

http/src/main/java/net/wildfyre/http/DataType.java
23

Read above

http/src/main/java/net/wildfyre/http/Request.java
54

Why this request header?

http/src/test/java/net/wildfyre/http/RequestTest.java
18

Token looks like "Token XXXXXXXXX"

src/main/java/net/wildfyre/api/WildFyre.java
11

Explain more

This revision now requires changes to proceed.Jul 20 2018, 11:37 PM
cache/src/main/java/net/wildfyre/descriptors/Descriptor.java
57

The warning is "the method doesn't need to be public", I suppress it because this is part of the public API (IntelliJ detects that the method is never used outside of the package, but doesn't know it's its purpose).

It feels like it would be redundant to add a comment about it every time it appears (which is probably going to be very often)... Do you want me to do it?

cache/src/main/java/net/wildfyre/descriptors/Internal.java
63

Because objects are removed/invalidated "after 5 minutes" for example.

cache/src/main/java/net/wildfyre/descriptors/User.java
206

In Java, it is standard to override toString, equals & hashCode. It's even expected from the standard API that you do it, more info

cache/src/test/java/net/wildfyre/descriptors/UserTest.java
54

Was needed in an earlier version, I'm gonna remove it

http/src/main/java/net/wildfyre/http/Request.java
54

So the server does not send HTML responses (by default, if none is specified, Java adds a "Accept: text/html" header)

http/src/test/java/net/wildfyre/http/RequestTest.java
18

In the API it just looks like XXXXX, the "Token " part is appended when you call Request.addToken(String) (see above)

  • Removed unneeded SuppressWarnings

@WyldBot do you want me to add a comment about SuppressWarnings everywhere (WeakerAccess will be suppressed pretty much everywhere) ?

There might be a way to suppress that one warning from the entire project, I'll have to check that.

cache/src/main/java/net/wildfyre/descriptors/Internal.java
63

What objects specifically?

cache/src/main/java/net/wildfyre/descriptors/User.java
206

Yes, in general, overriding concrete methods is a code smell.

Because the base method has a behavior associated with it that developers usually respect, changing that will lead to bugs when your implementation does something different. Worse, if they change the behavior, your previously correct implementation may exacerbate the problem. And in general, it's fairly difficult to respect the Liskov Substitution Principle for non-trivial concrete methods.

Your link states only what is required if you do override a function

  • Added user manual
  • Longer linting time, because it takes more than 15 seconds
  • Spaces instead of files in MarkDown files
cache/src/main/java/net/wildfyre/descriptors/Internal.java
63

Anything in the cache, currently only users but later posts, comments etc too

cache/src/main/java/net/wildfyre/descriptors/User.java
206

No, really, I'm serious, equals and hashCode exist in Java so they are overriden. In fact, some collections (such has HashSet or HashMap) will NOT work properly if you don't.

Also, overriding them the way I did it (only putting the ID for hashCode, etc) increases the performance of Java collections by a lot

explanation

More comments (i have to post something)

cache/src/main/java/net/wildfyre/descriptors/Internal.java
63

So if you're only caching items for 5 minutes, whats the point in caching them at all?
Especially when that data could change at any time.

cache/src/main/java/net/wildfyre/descriptors/User.java
206

Carry on

CLOVIS added inline comments.
cache/src/main/java/net/wildfyre/descriptors/Internal.java
63

The value can be tweaked afterwards, that's not a problem (there's a constant for it in the Internal class).

This revision is now accepted and ready to land.Jul 25 2018, 2:26 AM
This revision was automatically updated to reflect the committed changes.