Conversation
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/HttpResponseExtension.cs
Show resolved
Hide resolved
| public static async Task WriteError(this HttpResponse response, string errorMessage) | ||
| { | ||
| JObject message = new JObject {{"error", new JValue(errorMessage)}}; | ||
| JObject message = new() {{"error", new JValue(errorMessage)}}; |
There was a problem hiding this comment.
Shorter syntax for constructors. Type is already in the declaration of the variable so, no need to be redundant.
| public class Init | ||
| { | ||
| private readonly SemaphoreSlim _initSemaphoreSlim = new SemaphoreSlim(1, 1); | ||
| private readonly SemaphoreSlim _initSemaphoreSlim = new(initialCount: 1, maxCount: 1); |
There was a problem hiding this comment.
Shorter syntax and clearer parameters.
| string body = await reader.ReadToEndAsync(); | ||
| JObject inputObject = JObject.Parse(body); | ||
| if (!inputObject.ContainsKey("value")) | ||
| if (!inputObject.TryGetValue("value", out JToken? message) || message is not JObject valueObj) |
There was a problem hiding this comment.
Use pattern matching to assign valueObj right away.
| { | ||
| await httpContext.Response.WriteError("Missing main/no code to execute."); | ||
| return (null); | ||
| return null; |
There was a problem hiding this comment.
I am not sure why all return values are between parentheses. Is this enforced by the Scalariform plugin?
| bool binary = valueObj.TryGetValue("binary", out JToken? binaryToken) && binaryToken.ToObject<bool>(); | ||
|
|
||
| if (message["main"] == null || message["binary"] == null || message["code"] == null) | ||
| if (string.IsNullOrWhiteSpace(main) || string.IsNullOrWhiteSpace(code)) |
There was a problem hiding this comment.
These are checking only for white space because main and code are guaranteed to not be null.
| archive.ExtractToDirectory(tempPath); | ||
| } | ||
| } | ||
| using MemoryStream stream = new(Convert.FromBase64String(code)); |
There was a problem hiding this comment.
No need to nest usings anymore.
| try | ||
| { | ||
| string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync(); | ||
| using StreamReader reader = new(httpContext.Request.Body); |
There was a problem hiding this comment.
Let's be good citizens and dispose any unmanaged resources created by the StreamReader.
| } | ||
|
|
||
| object owObject = _constructor.Invoke(new object[] { }); | ||
| object owObject = _constructor.Invoke(Array.Empty<object>()); |
There was a problem hiding this comment.
No need to allocate an object to invoke the constructor.
|
I forgot to mention that I still cannot run the tests locally in my machine since I can only run openwhisk in standalone mode in a docker container and I don't know how to run the tests against it. I tested manually by running the container for this runtime and sending the base64 encoded tests to it in the expected JSON text. It seems to work fine that way. I found some issues while trying to test it in integration with openwhisk in standalone mode. Specially the array input and output cases. |
1 participant
I mentioned that this could be a good improvement on the code for this runtime in #91
I probably got carried away with some other changes not specific to enabling NRTs. I will explain my rationale for these and ask questions about other minor details in the code.