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

Refactor java-sdk #136

Merged
merged 4 commits into from Dec 5, 2022
Merged

Conversation

thomasdarimont
Copy link
Contributor

@thomasdarimont thomasdarimont commented Dec 5, 2022

  • Refactor Plugin
  • Make Plugin and Context AutoClosable to automatically free resources in TWR blocks
  • Add missing javadoc
  • Upgrade to Junit 5
  • Reorganize packages
  • Use more meaningful names
  • Introduce ExtismException
  • Make JSON serialization pluggable
  • Lower required java version to Java 17

Improves #117

@thomasdarimont thomasdarimont changed the title Refactor plugin Refactor java-sdk Dec 5, 2022
@thomasdarimont
Copy link
Contributor Author

@bhelx I just gave this another spin and refactored the library to be a bit. Hope this meets your expectations. :)

I think we could also lower the required Java version to Java 17.

@bhelx
Copy link
Contributor

bhelx commented Dec 5, 2022

Hey @thomasdarimont we seem to be simultaneously working on the same stuff today. Happy to take your changes many of them look better than mine. If you want to take on some of the rest of the work though let me know so I don't step on your toes. Happy to have you lead here! :)

@bhelx
Copy link
Contributor

bhelx commented Dec 5, 2022

Make it autoclosable to automatically free resources in TWR blocks

🥳

@bhelx
Copy link
Contributor

bhelx commented Dec 5, 2022

I think we could also lower the required Java version to Java 17.

yeah let's go as low as we can safely go!

@bhelx
Copy link
Contributor

bhelx commented Dec 5, 2022

Let me know if you want to resolve the conflicts or me. I think it's safe to say I prefer your changes.

@thomasdarimont
Copy link
Contributor Author

Sorry for the overlap. I'll try to merge it quickly.

- Make Plugin and Context AutoClosable to automatically free resources in TWR blocks
- Add missing javadoc
- Upgrade to Junit 5
- Reorganize packages
- Use more meaningful names
- Introduce ExitsmException
- Make JSON serialization pluggable

Improves extism#117
@thomasdarimont
Copy link
Contributor Author

@bhelx I just finished the merge. I reused most of your JavaDoc comments and added some convenience methods.
I also made the Context class AutoClosable to leverage automatic resource cleanup in TWR blocks.

I saw that you made some methods protected, for which I wasn't sure whether they are intended for internal usage or usage by subclasses. Feel free to adapt the code to your liking :^)

@bhelx bhelx merged commit a42b858 into extism:java-sdk Dec 5, 2022
@bhelx
Copy link
Contributor

bhelx commented Dec 5, 2022

Thanks again! Is there anything else you had planned on working on? I actually think it's pretty close to being good for a release. I'm gonna setup CI and publishing next. If you have anything you're interested in doing let me know.

@thomasdarimont
Copy link
Contributor Author

Thanks @bhelx for the quick turnaround :) I just added some additional remarks on the java-sdk PR.
I give the project another look towards the weekend :)

thomasdarimont added a commit to thomasdarimont/extism that referenced this pull request Dec 7, 2022
- Refactor Plugin
- Make `Plugin` and `Context` `AutoClosable` to automatically free
resources in TWR blocks
- Add missing javadoc
- Upgrade to Junit 5
- Reorganize packages
- Use more meaningful names
- Introduce `ExtismException`
- Make JSON serialization pluggable
- Lower required java version to Java 17

Improves extism#117
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.

None yet

2 participants