-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
250 precursor #293
250 precursor #293
Conversation
@bmuenzenmeyer Some more precursor work for issue #250 . Please review and merge. |
@bmuenzenmeyer |
Makes sense. If there's anything I can do to help (e.g. if any of my code is unclear) just shout :-) |
I haven't made time to review either yet - but agree this area of code is undergoing a lot of change and likely one of these two PRs needs to win out or consume the other. I'll appreciate any degree of review you both can do in that regard in the meantime. |
Sure. I have a bit of time this afternoon, so I'll take a closer look at the changes in this pull request. |
@james-nash Could you please hold off review of commit 695471c ? I'll be submitting something more up-to-date hopefully later today. Thanks. |
Sure thing. Just shout whenever it's ready. |
@james-nash @bmuenzenmeyer Ready for review. Notes on the param parsing are in the docblock to paramToJson. FYI, I added the JSON5 package and replaced the JSON object with it, for better error messaging on malformed JSON. Running |
@@ -13,6 +13,7 @@ | |||
var list_item_hunter = function () { | |||
|
|||
var extend = require('util')._extend, | |||
JSON = require('json5'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it might be more appropriate to name this JSON5
, so that people don't mistake it for the built-in JSON
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@james-nash Had to debate over this myself, and I'm still torn. @bmuenzenmeyer What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too fussed myself. However, if you were to change it to JSON5
then I'd suggest doing that throughout all files where you use that library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my naivety on JSON5 (it looks like a nice solution to a problem we've all faced), but it doesn't sit right with me. My biggest worry is downstream consumption of .json5 files (I'd also argue that things like trailing commas are not good form. Yes comments would be nice). If people get accustomed to writing these files, or perhaps worse, writing them as "invalid" pure .json files, they might shoot themselves or others in the foot. Is this fear overblown?
Another consideration in my rather scattershot comment is that it looks like they don't follow semver (yet), which could create problems.
... more thinking ...
Put simply, can you explain how adding JSON5 makes #250 (which PR references by name) and the parameter_hunter.paramToJson() method better?
I am an outsider on the specifics of this issue, and have no doubt that a lot of thought has been put into getting us this far, but I need your help in understanding if this is straying too far from the goal of this issue/fix. c8acb5a mentions better error handling, but in what way is it better than the existing try catch logic? http://codepen.io/bmuenzenmeyer/pen/66eb18de343ab1954abc53537fde09f0 has an example that is descriptive enough, coupled with a bit more logging on PL's part, would seem to do the trick in my mind.
Thoughts?
cc @geoffp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to note that the pattern parameter syntax is only vaguely defined here http://patternlab.io/docs/pattern-parameters.html#pattern-parameter-syntax , but since the examples have keys that are not double-quoted, it's definitely not pure JSON. It's JSON-ish :-)
I'd be in favour of keeping our parsing of the pattern parameter syntax quite lax, since it's conceivable that users might copy-paste chunks of JSON data (so then keys might be double-quoted) and, as per issue #145 it's desirable to allow HTML code to passed in as a value so allowing single- as well as double-quoted string values makes sense to me (saves you having to escape every quote around HTML attributes and stuff like that).
The changes in this PR which make use of JSON5 enable all of that, so I think they're worth keeping.
Since, I've been focussed primarily on the parsing of pattern parameters, which is in parameter_hunter.paramToJson()
, I'm not as familiar with the other code though. Perhaps @e2tha-e can comment on that.
FWIW, where proper JSON data is expected, I'd agree that it might be better to be stricter (or at least display warnings in the build output) so as not to encourage sloppy JSON code that may not be interoperable with other tools or PatternLab versions.
Ok, I've finished reviewing. It looks great! I see that you brought across my additional test cases, so this solution successfully fixes the same problems I was attempting to fix in my PR #292. As far as I'm concerned, #292 can be rejected and this PR accepted instead. |
@james-nash Thanks a million for the code review! I just pushed that typo-fix, and so am waiting on further review and merge. So hopefully issue #291 can be put to bed soon! |
@bmuenzenmeyer @james-nash I'll be offline on vacation for a few days, so don't mind the radio silence too much. Looking forward to be back at it when I'm back 😄 |
@e2tha-e: No worries. Enjoy your vacation! |
paramString = paramString.replace(/(^\s*[\{|\,]\s*)'([^']+)'(\s*\:)/, '$1"$2"$3'); | ||
//check if searching for a key | ||
if (paramString[0] === '{' || paramString[0] === ',') { | ||
paramString = paramString.substring([1], paramString.length).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found another instance of using an array instead of a number for substring()
's first argument. Must have missed that last time around - sorry!
@bmuenzenmeyer I fixed that typo pointed out by @james-nash . I've also renamed the instances of JSON5 to be clear that it is not the native JSON object. I'm happy to replace JSON5 entirely, but I don't think it detracts from Pattern Lab in any way. I don't think it by default encourages lax JSON in the actual JSON files. Unless you peak under the hood, how would you know? JSON5 does, however, provide much better error handling than native JSON. I initially experimented with JSON5 to see if it would it could help with converting params to JSON. I found that it wasn't necessary, but boy were those error-handling message helpful. As such, I left it in. Any thoughts or suggestions are welcome. |
@bmuenzenmeyer Here's an example of the difference between native JSON and JSON5 exception handling. It's from running parameter_hunter_tests.js: Native JSON:
JSON5:
In issue #250 , @vebersol and I mentioned we worked on enterprise Pattern Lab projects with hundreds of patterns. I know personally, that the "Unexpected token" syntax error was the bane of my existence. There's no way to lint parameters, and how was anyone expected to be able to quickly track down where that error came from? Again, strictly syntaxed JSON is a requirement for .json files because they are read by fs.readJSONSync(). JSON5 has nothing to do with those files. I also wouldn't mind revisiting JSON5 being a drop-in replacement for JSON in the few instances where it's used, in the same manner that fs-extra is a drop-in replacement for fs. |
I'm coming around to this. I made This test is probably outside the scope of the current issue/fix since it relies on |
I'm realizing more and more that users are taking PL Node into territory and scale I haven't yet experienced personally or professionally. And that there is merit to making the library more robust, even at the expense of some sense of "purity" I define in my head. Your contribution addresses a need, is thoughtfully proposed, thorough in its implementation, and covered with unit tests. What more could one ask for? I'll need to be careful to forward port this into Thanks! |
Addresses #250
Summary of changes:
Some edits to paramToJson in parameter_hunter.js required when parsing an enterprise Pattern Lab instance and a unit test for it.