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 Passed
Unit
No Test Coverage
Build Status
Buildable 713
Build 713: 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
35

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

39

What is this used for?

42

All three should probably be lateinit instead of nullable

48

Possibly broken Dokka link

58

Possibly broken Dokka link

84

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

85

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

129

Possibly broken Dokka link

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

132

Should be a setter on a token property

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

Should probably be object

85

Should be a property with a custom getter

85

Shouldn't throw an NPE

92

Should be a property with custom getter

92

Shouldn't throw an NPE

104

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–56

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

56

Public field requires documentation

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
35

True.

42

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

48

Probably

58

Probably, also probably was there before idk

84

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

85

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

129

Ah ok thanks

132

Yes ok

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

No!

104

Yeah

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

Nah, kotlin is clever enough.

56

Ugh, documentation!?!

73–74

I know. I researched it a lot

src/main/java/net/wildfyre/api/Internal.kt
42
85

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

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

Why?

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

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

247

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

292

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

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

Where does .kn come from?

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

Should be written:

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

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
65

!= false is meh

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

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
39

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
26

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
60

See KDoc in InvalidJsonException

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

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
65

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
40

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