Skip to content

Conversation

@wiitt
Copy link
Collaborator

@wiitt wiitt commented Feb 26, 2025

Solves #35
In this PR, I tried not to ruin the logic of throwing (logging) errors and kept it as it used to be.
However, I think that it might be better to modify a bit the way how we do it. I would probably replace this endCode stuff in a writing function (it was save_as_file, now it's response_to_file) with throwing errors in when an unexpected exception occurs and "skip" messages when a file is skipped. It was the other way around before (raising errors for unpermitted overwriting files and logging messages for errors), and now I moved overwriting errors to a different level, but still not sure that it belongs there. I'd better have "skip" messages on the level of file consumer (the deepest possible level) and raising errors in response_to_file.

@KristenMeyerONC
Copy link

Can you add Matlab version requirements to the documentation (perhaps also the readme of the repo as well)

@wiitt
Copy link
Collaborator Author

wiitt commented Feb 26, 2025

Weirdly enough, confluence says that I cannot even see the documentation page in wiki. Like I don't have permission when I'm logged in.

@KristenMeyerONC
Copy link

You can make a systems ticket and request permission to edit the page you need to in the wiki or I can do it for you

@KristenMeyerONC
Copy link

KristenMeyerONC commented Feb 26, 2025

Can you also add a version note to the GettingStarted documentation: https://github.com/OceanNetworksCanada/api-matlab-client/blob/main/doc/GettingStarted.html#L6

The comment in the HTML file implies that the file was auto-generated but I'm not familiar with how that was actually done... I personally would just edit the HTML

Also, have we confirmed that this works on R2018a? I'm seeing 2022b in https://github.com/OceanNetworksCanada/api-matlab-client/blob/main/info.xml#L9 (maybe that should be updated to R2022b? Might be why the file exchange has different version requirements.). You can probably test R2018a editing https://github.com/OceanNetworksCanada/api-matlab-client/blob/main/.github/workflows/matlab.yml and rerunning checks

@wiitt
Copy link
Collaborator Author

wiitt commented Feb 26, 2025

Now I see what you meant about the auto-generated documentation. Yes, I wasn't right that there are no docs. Idk, I think I even saw it at some point, but, when I wanted to get back to it, I chose the only "wrong" file (xml) in the docs folder. So, I decided that I the documentation, I saw before, was for Python.

Do you think we need "source code" sections in the docs? Wouldn't it be sufficient to replace them with links to files on github?

No, I didn't confirm the work on R2018a. I solely relied on wiki documentation and checked MATLAB versions which all newly used classes/functions were added at. I agree that it would be better to actually test the client on R2018a, especially if this haven't been previously done yet.

P.S. I'll create a ticket for getting permission to edit the wiki, thanks.

@wiitt wiitt force-pushed the issue-35-storing-response-files-in-memory branch from b94b0d0 to f84c368 Compare February 28, 2025 23:09
@wiitt wiitt force-pushed the issue-35-storing-response-files-in-memory branch from f84c368 to 0669de7 Compare March 3, 2025 21:59
@spencerwplovie
Copy link
Contributor

spencerwplovie commented Mar 3, 2025

No, I didn't confirm the work on R2018a. I solely relied on wiki documentation and checked MATLAB versions which all newly used classes/functions were added at. I agree that it would be better to actually test the client on R2018a, especially if this haven't been previously done yet.

Based on https://jira.oceannetworks.ca/browse/DMAS-82515, it looks like the DAQ team has gotten the client library to work on a few Matlab versions - based on the comments, as old as R2019a and as recent as R2024a. But this is for varying CL versions; v2.1.1 and v2.2.0. Still worth testing to make sure, just an FYI.

@KristenMeyerONC
Copy link

KristenMeyerONC commented Mar 3, 2025

Do you think we need "source code" sections in the docs? Wouldn't it be sufficient to replace them with links to files on github?

Oh I see. It looks like the doc HTML files are generated from the actual .m source code. I think Matlab is auto adding the "source code" sections.

Eg: https://github.com/OceanNetworksCanada/api-matlab-client/blob/main/doc/Onc.html
https://github.com/OceanNetworksCanada/api-matlab-client/blob/main/onc/Onc.m

However, GettingStarted does not have a corresponding .m file so maybe just edit the HTML to add the version info for that one?

@wiitt wiitt force-pushed the issue-35-storing-response-files-in-memory branch from 3cbffe2 to 2dd3c70 Compare March 4, 2025 18:57
@wiitt
Copy link
Collaborator Author

wiitt commented Mar 4, 2025

Dropped one commit from this PR and created #37 to address version compatibility issues in a separate PR.

@wiitt wiitt merged commit d9c0ce2 into main Mar 5, 2025
2 checks passed
@wiitt wiitt deleted the issue-35-storing-response-files-in-memory branch March 5, 2025 21:53
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.

Client stores response body in memory before writing them into file

4 participants