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

Closes #345, Introduces KustoRequest wrapper object and async methods #358

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

The-Funk
Copy link
Contributor

@The-Funk The-Funk commented Mar 26, 2024

Added

  • KustoRequest object

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:

   var client = ClientFactory.createClient(engine_connection_string, httpClientProperties);

   // Example 1:
   client.execute(new KustoRequest("my query"));
   
   // Example 2
   var myProperties = new ClientRequestProperties();
   var req = new KustoRequest("my query", myProperties);
   client.executeToJsonResult(req);

@The-Funk
Copy link
Contributor Author

I meant to open this in draft. If there is interest in the request section, I can annotate the methods in question as deprecated.

@The-Funk The-Funk changed the title Introduces KustoQuery wrapper object Introduces KustoQuery wrapper object, closes #358 Mar 26, 2024
@The-Funk The-Funk changed the title Introduces KustoQuery wrapper object, closes #358 Introduces KustoQuery wrapper object, closes #345 Mar 26, 2024
@The-Funk The-Funk changed the title Introduces KustoQuery wrapper object, closes #345 Closes #345, Introduces KustoQuery wrapper object Mar 26, 2024
@AsafMah AsafMah requested review from ohadbitt and AsafMah and removed request for ohadbitt March 31, 2024 11:15
@ohadbitt
Copy link
Collaborator

ohadbitt commented Apr 2, 2024

I dont think we want this to be on our public API , we try to maintain a fairly similar API in all our sdks

@The-Funk
Copy link
Contributor Author

The-Funk commented Apr 4, 2024

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.

@AsafMah
Copy link
Contributor

AsafMah commented Apr 7, 2024

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?

@The-Funk
Copy link
Contributor Author

The-Funk commented Apr 9, 2024

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.

@The-Funk The-Funk changed the title Closes #345, Introduces KustoQuery wrapper object Closes #345, Introduces KustoRequest wrapper object May 22, 2024
@The-Funk The-Funk requested a review from AsafMah May 22, 2024 00:19
@The-Funk The-Funk changed the title Closes #345, Introduces KustoRequest wrapper object Closes #345, Introduces KustoRequest wrapper object and async methods Jun 6, 2024
@The-Funk
Copy link
Contributor Author

The-Funk commented Aug 5, 2024

@ohadbitt @AsafMah Can either of you trigger CI tests to run on this for me? I am confident it passes.

@AsafMah
Copy link
Contributor

AsafMah commented Aug 6, 2024

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?

@The-Funk
Copy link
Contributor Author

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.

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