Abstract HTTP Client plugin support (Issue 49) #50
Conversation
- changed the queue cluster to take the abstract class instead of the handle directly - updated dequeue/enqueue to work with the abstract class - updated method calls for abstract class - added patch by file check/method - minor adjustment to the create handle file still needs a "default class" implementation, and some testing.
- finalized the create/init (used a similar pattern to the other branched) - added a pure "pre-init" that allows the addition of the abstract class to the http-client class/child of choice (so if someone has created a child, they can still use it). - pulled the header manipulation into a subVI so that it could be reused a couple of places - added a few tests - added tests for "base/default" "NI Advanced Client" and the "reqwest" library. NOTE: since reqwest is not in vi.lib it might have to be relinked... this is fine for this initial test PR, but will need to be adjusted before production
# Conflicts: # source/REST Client.lvproj # source/classes/REST Client/Create REST Client.vi # source/classes/REST Client/Destroy REST Client.vi # source/classes/REST Client/Flush Client Handles.vi # source/classes/REST Client/Get client handle.vi # source/classes/REST Client/HTTP Request.vi # source/classes/REST Client/New Client Handle.vi # source/classes/REST Client/REST Client.lvclass # source/classes/REST Client/Return client handle.vi
…ase-sensitivity - rename http-client-type input to "http-client-plugin (built-in)" to reflect default value means to use built-in - Create REST Client.vi checks if plugin input is null and uses built-in client, if that's the case.
|
@jyoung8711 looking good! I made a few tweaks, so far:
Let me know if you have any thoughts or concerns about these. I haven't done an exhaustive review of the PR -- I've basically just gotten it running and made those minor default behavior changes. |
but left the classes "on-disk" for now. two utilities were still in that class (Assert_Buffer_Is_File and Parse StatusLine) those have been moved to the REST Client class changed the Escape URL scope to public and moved to "Utilities"
Added alternate "httpbun" (which also apparently has better options for testing)
|
@jimkring Thanks for the feedback. A couple of thoughts:
(this logic was pulled out of the previous implementation, though I moved it from nested case structures into two select statements).
A few other things jumped out at me, and I'm making a new PR now which includes some additional changes that I'll detail there. |
|
Actually... I think by moving the branch forward on my github fork of this repo, it auto included those commits with this PR... Sorry I've never actually tried to do additional things to a PR before it was closed... so I guess the question is to you @jimkring, can you see the 3 commits I made this morning? Anyway the things I changed were:
I did not remove the client injection you added yet since that's still an open discussion point. |
|
@jyoung8711 yes, I see 3 commits in the PR branch that I can pull into my workspace--I think that's all working,
"LabVIEW HTTP Client" is explicit, and that's always better than implicit. The only benefit of shorter is that it fits in the context help without getting truncated by an ellipsis (...) -- please go ahead and make that change. Also, I noticed that all of the other controls/indicators have context help descriptions--we should probably add a description to the Oh, and I made that a Recommended input rather than Optional, since making it more visible helps educate users that this is a useful feature, rather than some hidden thing that they will never use.
What are the considerations we need to discuss? I'm OK removing that code I added, since it's redundant. What are your recommendations? |
I just wasn't sure if there was some specific reason you wanted it there. My preference is to leave it in the New Client Handle VI since that's called in the "Get Session.vi". That guarantees that we'll have a valid interface in there, no matter what -- though I can't think of a reason in the current version that we would be left w/out a valid class... I'll go ahead and remove the redundant code in the next commit, and make the other changes you've recommended. |
OK, the only reason that I put it in the constructor is that this was to me, the most obvious place for that decision/logic to exist and I didn't see it. Maybe there's a better set of reasons that it's in "Get Session.vi", but I didn't write that code. I'll take a look at Get Session.vi and how it's used and see how that sways my thinking. |
|
I see that this is done in the "New Client Handle.vi" which exists immediately downstream from where I added the almost identical block of code. So, please delete the redundant code that I added. |
|
Alright, pushed a small update where I updated the vi descriptions, and removed the redundant code. I added a reference to the "reqwuest" library, but also noted they could be found on VIPM... anticipating that it would be at some point in the near future. |
|
@jyoung8711 anything left to do on this PR in your opinion? |
|
I think we've definitely accomplished what the initial issue was targeting and got us ~90% of the way towards a release, if we decide to go in that direction... I think the only gaps to a release (with JUST this PR) are:
I considered making those changes, but since there are other things tagged with a 3.0.0 release target, I figured it would be best to hold off if either (a) it's decided NOT to go in this direction for the release or (b) it's useful to tackle any other outstanding issues. |
|
OK, thanks for confirming that @jyoung8711. I'll take inventory on the 3.0.0 release issues/tasks and keep you posted. I'd love to get some Caraya tests running in GitHub actions (on a Linux docker container)... |
jimkring
changed the title
|
FYI, I've renamed the title of this PR to be a little more explicit in describing the new feature/changes |
|
I'm going to merge this now and will update the VI Package Build release notes etc. in a different PR/commit. |
|
@jyoung8711 I just realized that at some point you merged develop (which included existing work on abstraction done by @francois-normandin) into My gut tells me (given that LabVIEW is nearly impossible to diff effectively) that we should base our Abstract HTTP Client improvements directly off of the latest If we do that, I might create a new |
|
@jimkring you may want to double-check that. I alluded to some confusion in my initial PR over this. The location of the "main" branch still had much of the initial work from @francois-normandin. I actually started this commit from the tag release, but I couldn't create a PR without it being attached to an existing branch... So I ended up doing a "merge" back into the develop branch, because THAT branch included the work you had done that moved the existing interface back to a "closer" version to the original, and something that matched a little nicer to what we were working towards. Sorry for the confusion, but the repo state was a little confusing since all the "current" branches were in some sort of state of partial completion. I'm fine with however you want to resolve this, and will be happy to take a look once it's done if you want another set of eyes on it. |
3 participants
This is in a pull request for #49.
While it's being merged into develop for cohesion reasons, it's really a branch off of the latest release tag #2.1.0
The latest commit on develop was a partial completion of the "interface migration" which this is essentially an alternate/competing solution for. Many of the VIs from that integration are still around (and will later need to be deleted/cleaned up), but this is sufficient for testing purposes, I think.
If you run from the main project, the project itself is "clean"
NOTE: There is a dependency on the lv reqwest repo. That may need to be relinked since that doesn't live in vi.lib (and I did not add it as a git submodule here).
The integration here works when tested against all 3 existing interface classes. There's a "simpletest.vi" that runs through all of the basic methods, and behaves as expected (though it's missing many tests). The next test would be against the .NET API, but that will take a little bit more work, and I'll try that a bit later.
I'll add a few additional comments in the initial issue.