Page MenuHomePhabricator

Big changes
Needs ReviewPublic

Authored by Hackintosh5 on Dec 4 2019, 9:56 PM.

Details

Reviewers
WyldBot
CLOVIS
Summary
  • Migrate everything to Kotlin
  • Improve cache management
  • Migrate to Ktor
  • Improve error handling (not tested thoroughly)
  • Break the API (v1.0 now?)
  • Nuke LazyMap
  • Allow deleting one's own posts
Test Plan

Unit tests are passing. I haven't run the SpotBugs yet, but it will surely flag some errors, none of which I'll understand

Diff Detail

Repository
rLIBWFJVA WildFyre Java Libary
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 714
Build 714: arc lint + arc unit

Event Timeline

CLOVIS requested changes to this revision.Dec 4 2019, 10:40 PM
CLOVIS added inline comments.
.idea/jarRepositories.xml
1 ↗(On Diff #988)

Why do you need this file? It looks like internal stuff that IDEA can get from Gradle anyway, so shouldn't be necessary

src/main/java/net/wildfyre/api/Internal.kt
34

The Dokka link should probably be [init] and not [.init]

38

What is this used for?

41

All three should probably be lateinit instead of nullable

47

Possibly broken Dokka link

57

Possibly broken Dokka link

83

This method is not necessary anymore thanks to all attributes having their automatically generated getters in Kotlin

84

Probably shouldn't throw an NPE. At least use a custom error message to explain that the Internal wasn't initialised correctly

128

Possibly broken Dokka link

If you run Dokka (there should be a Gradle task), it should report all of these as warnings

131

Should be a setter on a token property

src/main/java/net/wildfyre/api/WildFyre.kt
25

Should probably be object

84

Should be a property with a custom getter

84

Shouldn't throw an NPE

91

Should be a property with custom getter

91

Shouldn't throw an NPE

103

The documentation probably should make it more clear what the difference between clean and clear is

src/main/java/net/wildfyre/areas/Area.kt
55

Public field requires documentation

55–56

internal is a keyword in Kotlin, you probably shouldn't name a variable like that

73–74

It kinda sucks that accessors can't suspend. Well, I guess there's no choice.

This revision now requires changes to proceed.Dec 4 2019, 10:40 PM
.idea/jarRepositories.xml
1 ↗(On Diff #988)

I probably don't need it.

src/main/java/net/wildfyre/api/Internal.kt
34

True.

41

No, because they don't have to be initialised every time we call init(). And kotlin won't allow us to check if they are null, if they are lateinit, so we would pointlessly initialise new objects

47

Probably

57

Probably, also probably was there before idk

83

It's just a helper because I prefer () to !!. Can be removed sure

84

No, it's an internal error so it should throw an internal exception.

128

Ah ok thanks

131

Yes ok

src/main/java/net/wildfyre/api/WildFyre.kt
25

No!

103

Yeah

src/main/java/net/wildfyre/areas/Area.kt
55

Ugh, documentation!?!

55–56

Nah, kotlin is clever enough.

73–74

I know. I researched it a lot

src/main/java/net/wildfyre/api/Internal.kt
41
84

I disagree. If a user has this problem, they should know why it happened.

src/main/java/net/wildfyre/api/WildFyre.kt
25

Why?

src/main/java/net/wildfyre/areas/Area.kt
90

Function is called spread but returns reputation, smells like a copy paste mistake

246

Shouldn't throw an NPE (the next step ignores null values anyway)

291

Shouldn't throw NPE (next step ignores null values anyway)

src/main/java/net/wildfyre/areas/Areas.kt
59

Where does .kn come from?

src/main/java/net/wildfyre/descriptors/Descriptor.kt
52

Should be written:

return lastUsage?.let {
    currentTime - ...
} ?: false
src/main/java/net/wildfyre/http/MultiPartContent.kt
47

You're using \r\n everywhere, make it a const val instead of a magic value

src/main/java/net/wildfyre/http/Request.kt
52–54

Put the default parameter at the end and use @JvmOverloads

src/main/java/net/wildfyre/utils/InvalidJsonException.kt
64

!= false is meh

src/test/java/net/wildfyre/areas/AreaTest.kt
39

Seems unnecessary

Hackintosh5 marked 2 inline comments as done.
  • Fix stuff that got asked for
Hackintosh5 marked 10 inline comments as done.
  • Little things
Hackintosh5 added inline comments.
src/main/java/net/wildfyre/api/Internal.kt
38

It means that the individual classes don't have to maintain a CacheManager, they can use a one-liner instead

src/main/java/net/wildfyre/api/WildFyre.kt
25

Singletons are bad. I spent literally hours refactoring them away. What if the client app wants to access another account in the background, to get notifications for an alt account?

src/main/java/net/wildfyre/areas/Areas.kt
59

See KDoc in InvalidJsonException

src/main/java/net/wildfyre/http/MultiPartContent.kt
47

but it is a magic value, and it's never going to change. This is almost a verbatim paste from the ktor docs

src/main/java/net/wildfyre/utils/InvalidJsonException.kt
64

It's suggested by JetBrains as better than checking for null and then checking isNull. It's probably optimised away anyway

src/test/java/net/wildfyre/areas/AreaTest.kt
39

It fails otherwise. connectToTestDB returns a value but JUnit tests cannot (or they won't run)

Hackintosh5 marked 4 inline comments as done.
  • Dokkaaaaaaaaaaaaargh

This is stale, and cannot be used since we migrated to GitLab some time ago