Closed The-Funk closed 2 months ago
I meant to open this in draft. If there is interest in the request section, I can annotate the methods in question as deprecated.
I dont think we want this to be on our public API , we try to maintain a fairly similar API in all our sdks
I dont think we want this to be on our public API , we try to maintain a fairly similar API in all our sdks
@ohadbitt the reason I really liked this is just that a wrapper object like this really shortens your method signatures and reduces the number of methods you need to maintain. The object acts as a simple container that can hold whatever needs to be sent between methods, which in the long run saves you from needing to add more methods with different signatures when that context undergoes changes. For instance, when you need to add an enhancement you can just add the additional context to the wrapper, add the logic to the method implementation that consumes the wrapper, and then add your documentation. If you package JavaDocs the documentation is just some comments over some getters/setters.
If you have a chance, can you check out my latest comment over on issue #249 ? I was hoping to preface the async implementation with this, but I don't mind following one of the other potential paths if you have a preferred implementation instead. I get wanting to keep the SDKs aligned for sure.
We actually want to remove some of the overload in the next version. This week Me and Ohad will brainstorm on the async design, and we'll see how the API will change. Maybe this class should be used internally?
We actually want to remove some of the overload in the next version. This week Me and Ohad will brainstorm on the async design, and we'll see how the API will change. Maybe this class should be used internally?
I think unless you exposed the container object like this one on the API somewhere, you would have to chain/overload methods. That said, if it's important to chain methods to keep the APIs aligned, I don't mind writing some extra boilerplate. Let me know what y'all come up with on the async implementation. I've been messing around with it. There's a lot of work no matter which way we go.
@ohadbitt @AsafMah Can either of you trigger CI tests to run on this for me? I am confident it passes.
Don't know since when it's a thing, but github won't run the trigger on PRs with merge conflicts - https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request
So can you fix those first?
Don't know since when it's a thing, but github won't run the trigger on PRs with merge conflicts - https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request
So can you fix those first?
Since the two branches are so different and the diffs are really hard to read , I'm probably going to create a new branch and manually verify each file individually. I don't want to throw out anyone's commits but there's major changes between this branch and main, so it probably makes sense to start fresh and check each file by hand. Otherwise I'd have to cherry pick commits, and that doesn't sound fun.
Added
Why: Adding this object allows for extensibility without changing method signatures or chaining methods until you reach an implementation, which should reduce the surface of the library and make the code easier to read. Currently to execute a command you must pass each property needed individually. Sometimes this results in a lot of method chaining, which can get pretty verbose and hard to track. Additionally, when new options are made available to the user, more chains are needed to account for this, which means a lot more verbosity.
Request: Deprecate the old methods that take each parameter individually, including the command and management methods, in favor of a single execute method that takes a KustoRequest object. This is a big change for users, however it should be a relatively simple one, and if the methods are annotated as being deprecated for a time prior to removal, this will give users time to migrate to using the KustoRequest object.
New usage: