Carpet

Carpet

2M Downloads

[Structure] Moving API things to an API package

altrisi opened this issue ยท 3 comments

commented

When possible, things could be moved into an API package, something like carpet.api, to make clear those are things that are expected to be used externally and are less likely (or guaranteed not until deprecation) to change.

Things that should be moved IMO:

  • The entire carpet.settings package (maybe with some things being package private). Could be easily added to my #1330 PR to cause less disruption, and some implementation specific things could be skipped, made package private or reworked externally without exposing them
  • CarpetExtension
  • carpet.utils.Messenger
  • Some things in TickSpeed, maybe, to allow querying it at least
  • Things around carpet.logging (not too familiar with that package, not sure what should stay there)
  • Network handlers maybe? (not sure if those are supposed to be APIs)

Things out of scope (of this specific issue at least):

  • Scarpet related APIs
    More complex subject
  • CarpetServer's utilities
    I'm thinking of maybe moving those to a different class if I ever get to make CarpetServer have instances directly as a field of MinecraftServer and make it implement CarpetExtension. Edit: Thinking of minecraft_server and settingsManager fields and manageExtension method

Obviously all this changes for things that are considered used would be kept temporarily as @Deprecated(forRemoval = true) before they are finally removed in a later update, to keep binary compatibility.

Thoughts? If this is accepted I'll modify #1121 to take this into account.

commented

Ah ok, I misunderstood the point, thanks for clarifying. I thought it was to clean up the code itself. Then in that case I agree with you about the Scarpet stuff, I was just voicing my opinion on the events thing as well.
With regards to the other stuff, I completely agree with carpet.settings, tho for Messenger maybe we could instead put like a Output class which dealt both with sending messages to chat, as well as via logger, so we could wrap those to up at once?

commented

You aren't understanding the point of this issue. It is to move API related classes into an api package instead of being scattered around the code without knowing what is and what isn't API, so you don't have to fear about binary incompatibilities when depending on Carpet code.

Also if I understood correctly scarpetApi in CarpetExtension is not fully supported API (the method in the interface yes, the actual use of the Expression no), you should be using AnnotationParser and the rest of methods and classes around carpet.script.annotation to add Scarpet functions normally.

And yes events are... not really extensible nor easy to use, and let's not dive into entity events, I don't currently know how would you extend those...

Back to Scarpet because I somehow kinda changed subject, the problem is that if you wanted to move things into an API package you'd have to either separate part of the script package into such api package, or have an api package inside script, that actually already exists and is used for something else. Not only that, as a Scarpet extension you likely need access to Value and its subclasses, InternalExpressionException, Context, Fluff, and possibly other things around there, so it's more difficult to decide what to move and if things should be moved, what parts are API and how to encapsulate things from those classes that have to be called but aren't API.

commented

In terms of Scarpet API specifically, I don't rly see what's to be done much there. Cos adding functions is as simple as in scarpetApi() method using the provided Expression expr variable and doing expr.addWhatever(), or with the newer stuff that you added, which I don't quite understand. And especially there, your documentation is incredibly thorough, so I don't think it's needed.
The only thing which I would say needs to be updated or made more extension-programmer-friendly would be the event stuff, cos currently it's a nightmare (see this pr for an example). I think it could be easily done, especially with the new event handler stuff. In fact, if it's accepted, I can do it myself.