Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed Apache Http Client and Rest #2

Merged
merged 3 commits into from
May 18, 2020
Merged

Removed Apache Http Client and Rest #2

merged 3 commits into from
May 18, 2020

Conversation

ctadlock
Copy link
Collaborator

@ctadlock ctadlock commented May 13, 2020

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: #3

ctadlock and others added 2 commits May 12, 2020 21:15
@ctadlock ctadlock requested a review from edvin May 13, 2020 04:49
@ctadlock ctadlock added this to the 2.0 milestone May 13, 2020
@ctadlock ctadlock changed the title removed Apache Http Client and Rest Removed Apache Http Client and Rest May 13, 2020
@edvin
Copy link
Owner

edvin commented May 14, 2020

Why did you remove the JsonModel related functions?

@ctadlock
Copy link
Collaborator Author

@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
Copy link
Owner

edvin commented May 15, 2020

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
Copy link
Collaborator Author

@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.

  • / (README.md, CHANGELOG.md, LICENSE)
  • src/ (parent folder for all source)
  • src/tornadofx2 (tornadofx2 core)
  • src/tornadofx2-json (json extensions)
  • src/tornadofx2-rest (rest extensions)
  • docs/ root for documentation

@edvin
Copy link
Owner

edvin commented May 17, 2020

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 /?

  • / (README.md, CHANGELOG.md, LICENSE)
  • tornadofx2 (tornadofx2 core)
  • tornadofx2-json (json extensions)
  • tornadofx2-rest (rest extensions)
  • docs/ root for documentation

Alternatively, a modules root perhaps?

@ctadlock
Copy link
Collaborator Author

@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.

  • / (README.md, CHANGELOG.md, LICENSE)
  • src/ (parent folder for all source)
  • src/tornadofx2 (tornadofx2 core; root for pom.xml)
  • src/tornadofx2/src (tornadofx2 core; code)
  • src/tornadofx2-json (json extensions; root for pom.xml)
  • src/tornadofx2-json/src (json extensions; code)
  • src/tornadofx2-rest (rest extensions; root for pom.xml)
  • src/tornadofx2-rest/src (rest extensions; code)
  • docs/ root for documentation

@edvin
Copy link
Owner

edvin commented May 18, 2020

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

@edvin edvin merged commit 1cf504d into edvin:master May 18, 2020
@ctadlock
Copy link
Collaborator Author

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
Copy link
Owner

edvin commented May 18, 2020

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

@ruckustboom
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants