Page MenuHomePhabricator

Convert WildFyre.java to Kotlin
AbandonedPublic

Authored by Hackintosh5 on Feb 3 2019, 11:17 AM.

Details

Summary

Convert WildFyre.java main API to Kotlin

Starts to fix T272

Test Plan

Check tests pass (done manually due to arc being broken)

Diff Detail

Repository
rLIBWFJVA WildFyre Java Libary
Branch
arcpatch-D214
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 570
Build 570: arc lint + arc unit

Event Timeline

Hackintosh5 created this revision.Feb 3 2019, 11:17 AM
CLOVIS requested changes to this revision.Feb 3 2019, 11:31 AM
CLOVIS added reviewers: CLOVIS, WyldBot, Info-Screen.
CLOVIS added a subscriber: CLOVIS.

You removed the "WildFyre" class...

If you really want to make this one, be ready because I'm going to be very picky, there are a lot of things to do here

This revision now requires changes to proceed.Feb 3 2019, 11:31 AM

This is (from reading the Kt docs) the "correct" way... It's true, it would break existing clients except there is no-one using this library yet...

CLOVIS added a comment.Feb 3 2019, 3:18 PM

This is (from reading the Kt docs) the "correct" way... It's true, it would break existing clients except there is no-one using this library yet...

I don't like having global methods though, it's just not clean

In D214#5119, @CLOVIS wrote:

This is (from reading the Kt docs) the "correct" way... It's true, it would break existing clients except there is no-one using this library yet...

I don't like having global methods though, it's just not clean

Mhm, I'll move to a companion object tonight then.

CLOVIS added a comment.Feb 3 2019, 9:11 PM
In D214#5119, @CLOVIS wrote:

This is (from reading the Kt docs) the "correct" way... It's true, it would break existing clients except there is no-one using this library yet...

I don't like having global methods though, it's just not clean

Mhm, I'll move to a companion object tonight then.

Move it to a regular object, that'll be cleaner.

CLOVIS added inline comments.Feb 3 2019, 9:12 PM
src/main/java/net/wildfyre/api/WildFyre.kt
59

This could be a val with a custom getter (once everything move in an object)

CLOVIS added a comment.Feb 3 2019, 9:13 PM

When you create a diff, please remember to add "Fix [task]" in the summary. I'll do it this time, but try to remember for next time.

CLOVIS edited the summary of this revision. (Show Details)Feb 3 2019, 9:14 PM
CLOVIS edited the summary of this revision. (Show Details)

You will also need to convert Internal, in the same way

Hackintosh5 updated this revision to Diff 754.Feb 4 2019, 9:33 AM
  • Refactor WildFyre.kt to use a Java class and companion object
Harbormaster completed remote builds in B556: Diff 754.
Hackintosh5 updated this revision to Diff 755.Feb 4 2019, 9:36 AM
Hackintosh5 marked an inline comment as done.
  • Fix accidental upload of changed from another Diff
CLOVIS requested changes to this revision.Feb 5 2019, 6:43 AM
CLOVIS added inline comments.
src/main/java/net/wildfyre/api/WildFyre.kt
10

You can do this with "object WildFyre"

This revision now requires changes to proceed.Feb 5 2019, 6:43 AM
Hackintosh5 updated this revision to Diff 768.Feb 5 2019, 9:56 AM
  • Simplify companion object syntax
Harbormaster completed remote builds in B570: Diff 768.
Hackintosh5 marked an inline comment as done and an inline comment as not done.Feb 5 2019, 9:57 AM
CLOVIS requested changes to this revision.Feb 5 2019, 6:04 PM

Looks good to me so far, but you need to do the Internal class too (in the same way).

Please tell me if you have a better name for it, "internal" doesn't record what it's for clearly enough in my opinion

This revision now requires changes to proceed.Feb 5 2019, 6:04 PM
In D214#5248, @CLOVIS wrote:

Looks good to me so far, but you need to do the Internal class too (in the same way).
Please tell me if you have a better name for it, "internal" doesn't record what it's for clearly enough in my opinion

In Slack I was told to skip Internal because it has complicated Java things that I don't understand, which need to be converted to Kotlin that I also don't understand.

CLOVIS added a comment.Feb 6 2019, 7:10 AM

Oh. Okay, remove the "fixes t..." from the description here, otherwise pantocrator will close the task. I'm probably going to test all your diffs this week end.

Are you experienced with Git? If so, you can try to clean the commit history (unlike other WildFyre projects, we keep all the commits here)

In D214#5270, @CLOVIS wrote:

Oh. Okay, remove the "fixes t..." from the description here, otherwise pantocrator will close the task. I'm probably going to test all your diffs this week end.

Okay

Are you experienced with Git? If so, you can try to clean the commit history (unlike other WildFyre projects, we keep all the commits here)

I know how to squash with a rebase if that's what you mean?

I know how to squash with a rebase if that's what you mean?

git rebase -i is even better if you're familiar with it... it doesn't matter too much though.

Hackintosh5 abandoned this revision.Jul 2 2019, 12:08 PM