Closed jan-herout closed 3 years ago
Hmm, not sure why this fails, it has nothing to do with the change.
FAIL
FAIL github.com/CloudyKit/jet/v6 0.072s
? github.com/CloudyKit/jet/v6/examples/asset_packaging [no test files]
? github.com/CloudyKit/jet/v6/examples/asset_packaging/assets/templates [no test files]
? github.com/CloudyKit/jet/v6/examples/todos [no test files]
? github.com/CloudyKit/jet/v6/jettest [no test files]
I have read the ticket second time, and my attempt is something different then what was asked for. Do not waste your time, I might put it here in the end. I will sleep on it.
OK, I had to try again. Please let me know, what do you think - could this be the solution to #187? Thanks!
Regards,
Jan.
The changes look to me like you addressed #111, not #187?
To be honest, I am not sure why comparison against master contains also changes implemented by 4beb915 and f67928d - that is confusing.
I assume you didn't pull the newest master of CloudyKit/jet into your fork before starting with the new PR? The hashes are different from your original commits since in this repository, we use a "rebase, then merge" strategy that avoids merge commits. A side effect is that the commits are rewritten with the merging person added to them as commiter (in addition to you as the commit author). See here for example: https://github.com/CloudyKit/jet/commit/f67928da339229e4ccb9b198a6428b8f077cf1c9. This means your fork has a commit with the same changes as f67928d, but it has a different hash and so Git doesn't know the code is the same in your fork and this repo, so the changes are shown in the diff.
Assuming you really did mean to address #111, I added a comment there that is helpful feedback to this PR at the same time: https://github.com/CloudyKit/jet/issues/111#issuecomment-771609451
I don't expect all of that to be done in this PR, but I'd like whatever changes you are able to make to go in that direction, maybe start with just a simple dumpVariables function without any parent scope walking here and we can expand to all the other stuff I laid out in the comment later.
As for the failing tests, it seems you created devdump.jet on Windows with \r\n line endings, when Jet only produces \n line endings, so the expected output doesn't match what dump prints: https://ci.appveyor.com/project/CloudyKit/jet/builds/37566590/job/jo1ne5y95yyqs0jd#L880
Thanks for both comments. When possible, I will look at that. And indeed, this should be related to #111 - my bad. And your Windows comment is spot on.
All right. I would appreciate your feedback. Let's assume that this is the alfa version. Also, documenation is not entirely up to-date, and newly added functions under dump.go
probably deserve a few well placed comments.
I did not manage to commit the test case template ins uch a manner, that EOL convention would be Unix - sorry for that - however, the test case accounts for that, and runs both on Windows and Linux correctly.
Functions used in the code are "not panicky", I did not like the idea of that.
Regards,
Jan
EDIT - output looks like this: https://github.com/jan-herout/jet/blob/7d9d95a2e35a1f594a2f7c266636e3322eabdf1e/testData/devdump.jet#L27
Thanks for the review, will look at these changes most likely yesterday. This is becoming a valuable experience.
Let me know should you find anything else. Regards,
Jan
Very sorry it took me so long to get back to you!
The following is - perhaps naive? - attempt to satisfy requirement described in issue #187. Let me know what you think - Jet internals are mystery to me, at least for now.
To be honest, I am not sure why comparison against master contains also changes implemented by 4beb91506060bd8900ec0f7d42e401d21300350f and f67928da339229e4ccb9b198a6428b8f077cf1c9 - that is confusing.
Regards,
Jan