Use dedicated class and types for server communication#64
Conversation
| <osc-data:key>${params.command}</osc-data:key> | ||
| <osc-data:key>${params.project}</osc-data:key> | ||
| <osc-data:key>00atf/${params.filename}</osc-data:key> | ||
| </osc-data:keys>`; |
There was a problem hiding this comment.
One thing I had planned to do but I think I never did was to use templates for the xml used in the server communication. I imagine there's something like jinja2 for Typescript.
There was a problem hiding this comment.
So you could have an XML template file for each time of message, or chunk of message, and then fill it in with the given parameters.
There was a problem hiding this comment.
Good idea, I'll make another issue for that! Especially because the indentation is confusing here.
| log('error', `Error during communication with server: ${err}`); | ||
| vscode.window.showErrorMessage( | ||
| "Problem getting response from server, see log for details."); | ||
| return; // Should return an empty result? |
There was a problem hiding this comment.
No need for an empty result to be returned I think. You're catching the errors and the ServerResults are returned via a Promise (which I don't quite understand yet!). What would you use an empty result for?
There was a problem hiding this comment.
The Promise represents a result that's not finished computing yet. At some point we'll need to either return a result, or say that there's an error. Maybe the right thing is to throw an error here, and let the caller show the message etc. I have trouble explaining it because I don't fully understand promises either.
| Please contact the Oracc server admin to look into this problem.`; | ||
| case 'run': | ||
| if (attempts < max_attempts) { | ||
| log('info', 'Server working on request.'); |
There was a problem hiding this comment.
Does this mean that the user will see "Server working on request" as many times as attempts there are? I don't know how many of those it normally takes until the server finished processing the request, but I'd log the attempt number in the debug log maybe, and just the "Server working on request" for the users to see just once. Or even not at all.
There was a problem hiding this comment.
I was going by what Nammu does (it prints every time on the console), but you're right that it doesn't show it on the main window. This will only show up to ten times (we'll time out after that).
I wanted to have something that shows that it's not stuck, but your suggestion sounds better, thanks!
raquelalegre
left a comment
raquelalegre
left a comment
There was a problem hiding this comment.
I think this improves the previous design quite a bit! It's easy to read and understand, although maybe it's easier for me as I know how it works! The only thing I'd consider which might make things easier to understand is to use templates for the XML text rather than chunks of formatted text inbetween lines of code. This is something I wanted to have done in Nammu but never found the time to do. Just a suggestion! It reads fine as it is.
ageorgou
force-pushed
the
ag/refactor-server
branch
from
7e556ea to
3912809
Compare
I think mime.ts makes more sense under client, where it's being called from. So now server only has the user-facing stuff about communicating to the server, not the behind-the-scense SOAP client.
ageorgou
force-pushed
the
ag/refactor-server
branch
from
c9dc734 to
d2c24b8
Compare
2 participants
Not fully convinced this is worth it but interested in feedback, especially about readability and whether the structure makes sense.
The general plan was:
SOAP_clientclass.mime.tstoclient/fromserver/Use theDeletedHTTP_requestclass for some of the functionality that's currently inmime.ts(or some ofSOAP_clienttoo)HTTP_requestMy thinking is that:
"val"instead of"atf").I'm not sure about
HTTP_request, because the two types of requests we create do different things, so a shared base may be tricky. I don't know if putting that in a class (rather than functions) is going to be a better structure.