search

edvin / tornadofx2

TornadoFX 2.0
Apache License 2.0
155 stars 41 forks source link

Removed Apache Http Client and Rest #2

Closed ctadlock closed 4 years ago

ctadlock commented 4 years ago

This is being removed from the core of tornadofx; should be added later as a separate and optional item. Reasoning is that not all apps will want to use this specific Http client.

This is for: https://github.com/edvin/tornadofx2/issues/3

edvin commented 4 years ago

Why did you remove the JsonModel related functions?

ctadlock commented 4 years ago

@edvin Those json model functions are in the Rest.kt file. Not actually sure why they are there in the first place. I started by deleting that file and fixing compilation errors.

Note that the next MR I was going to submit was removing the Json dependency. That one is more complicated as it deeply integrated with the configuration code.

I also think we should remove the specific dependency on FontAwesome, not the core code that supports it though. This causes us an issue as we pay for FontAwesome and have a separate version than what is included in tornadofx. My belief is that tornadofx should enable the easy use of font icons, but not have specific support for any of them in core.

We should also decide where this non-core code should go. This repository but a separate project? If so, how does that effect deployment? Separate repository?

image

edvin commented 4 years ago

I agree removing the FontAwesome dependency is a good thing. The json dependencies in Configuration and other places could be added to the json module as extension functions, couldn't they? A separate project for each of the modules we extract sounds like a good way to go.

ctadlock commented 4 years ago

@edvin Want to be really clear on this. Does this structure/naming work for you? Each module would be published separately.

One repository tornadofx2; multiple maven modules for each sub module including core.

edvin commented 4 years ago

The current /src root now is there because of the Maven convention, not sure it is a good idea to use the same for modules. What about just putting the modules in /?

Alternatively, a modules root perhaps?

ctadlock commented 4 years ago

@edvin I could have been more clear. Here is the full example. Personally I like all modules to be siblings of each other, and having all source code under a main src folder which allow for other things like docs.

edvin commented 4 years ago

I understood what you meant :) OK, I'm fine with it!

ctadlock commented 4 years ago

OK! This should enable us to make some progress on this.

Wasn't expecting you to merge this in just yet. Ill put in other PRs for the folder restructure and other modules.

edvin commented 4 years ago

Sorry about that :) I'll leave the merge to you from now on, will just comment on the merge requests.

ruckustboom commented 4 years ago

@ctadlock For future note, you can mark your PRs as drafts if they're not actually ready to be merged.