JKI-JSON-Serialization
JKI-JSON-Serialization copied to clipboard
JKISoftware
Feature/filename in error msg
Create new optional input for defining the source of the JSON String being parsed, so that the filename/URL can be inserted into the error message.
I intend to create another branch for improving the parsing error messages to include snippets of the failed json with '-------^' notation to point to the point of failure. This improved error reporting is not dependent on the code changes in this PR, but may be touch some of the same files, not sure. I would not be entirely opposed to including both changesets into a single PR, however, the two improvements are not technically related.
@nate-moehring I love adding file/url source information to error messages. As I'm reviewing this, a question popped into my head about keeping related work together (cohesion) and I wondered if inserting the file path information at the same "level" as the file read would make sense. What are your thoughts about this?
Related question: As a user of the JSON Deserializer how will I know what this new input does and how/when I should use it? (Asking because I didn't see mention in the VI Description nor was it apparent from the control name since path/url isn't data needed for parsing the JSON).
As I'm reviewing this, a question popped into my head about keeping related work together (cohesion) and I wondered if inserting the file path information at the same "level" as the file read would make sense. What are your thoughts about this?
Natural question, and I even stated essentially the same thing in my original email to you about this PR. (I thought it was in a comment field somewhere, but I guess not, my apologies.) I agree this is kind of breaking some SOLID rules, however, I have two reasons that motivated the change as is:
- My main reason for doing it is, as far as I can tell, Read Configuration.vi is not actually included in a package anywhere that I can find, and certainly not the palette. Am I wrong? As a result, users have been developing their own versions of reading JSON from config files.
- Second, what about the scenario where the JSON blob actually came from a REST request, not a file. It would still be nice to give some kind of indication where it came from even though a filename is no longer applicable.
Related question: _As a user of the JSON Deserializer how will I know what this new input does and how/when I should use it?
You're right, I should have said something about it in the VI Description of Unflatten From JSON String.vi, though I think I did put something in the Control's Description about it's use.
Hi @nate-moehring. Great points. I wonder if it would make sense to generalize the VI that appends file path/url the error string. Here's the direction of my thinking:
This could be converted into a subVI that is then called by VIs that read or write the file, as such.
Here's how this could be further improved(?)
Wow, thanks for all the time you put into thinking about alternative implementations. I guess this is a good solution IF the new subvi is added to the palette.
This could be converted into a subVI that is then called by VIs that read or write the file, as such.
This turns out not to be useful because the Write File primitive already includes the filename in the error, at least for some error codes...
Alright, I've made the requested changes. I also removed the wrapper tests VIs, because it seems like Caraya was not correctly auto-removing sub-tests from the list of discovered tests. Each test was getting run several times. The best code is no code, let the tool find the tests.
I guess the .vipb is stored in an internal JKI repo?
Alright, I've made the requested changes. I also removed the wrapper tests VIs, because it seems like Caraya was not correctly auto-removing sub-tests from the list of discovered tests. Each test was getting run several times. The best code is no code, let the tool find the tests.
I guess the .vipb is stored in an internal JKI repo?
The .vipb is here: src/JSON Serialization.vipb
If you're willing to do a test build and see if it works well, that would be great!