Refactored your PR #1
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These property changes used to treat atoms that start with a colon the same way that lists that are a quotation are treated now.
Relics from the past shall haunt us no more.
Since it doesn't *always* unwrap a quote; just when it detects one.
Just for easier reading.
The `otherwise`-branch covers the same case.
It's safe to `@compile` things multiple times (it's a no-op when done again), and this `fold1` is neater than a loop. Seems like `coerce-property` will always fail its conditional unless passed the `string-is-computed` argument (in which case it's a no-op), so the call was redundant.
That error checking was duplicated a lot. This error message should be specific enough.
It was pretty confusing to me what coerceProperty was doing. Because it's been reduced to just a single conditional at this point, might as well write it as a conditional instead wherever it's used: that requires holding less state in mind when reading.
There were lots of lines of code between where this was defined and the *one place* where it was used, so I inlined it for readability.
That `i` parameter standing for the argument index currently being processed is non-descriptive and clutters up the place. It's neater to prepend the "there was a problem with argument #i" type of error text in a central place, and have the leaf conditionals just throw an error with the details. Also, few of these error cases were being tested.
In the previous commit, generic JavaScript errors (like TypeError, etc) would wrongly have the "Unexpected object macro argument #i: " text prepended to their message. That goes away by throwing a specific error type.
Reduces duplication and guards against future changes to how the function macro is implemented.
That's already done by the function macro.
Similarly to set/get, this keeps all the function-constructing stuff in one place, including error checking.
It's unnecessary: the [estree spec for ES5][1] doesn't specify it, and [the estree spec for ES6][2] only specifies it for nodes with type "ArrowFunctionExpression", which we aren't using and which can't be used as getters and setters anyway. [1]: https://github.com/estree/estree/blob/master/spec.md [2]: https://github.com/estree/estree/blob/master/es6.md
This does mean we're blocked on estools/esvalid#7 if we want safety.
Consider an ES6 object literal with a method that has empty arguments
and an empty body:
{
d() { }
}
Before this commit, that would intuitively have been written as
(object ('d ()))
which failed because the ('d ()) list has 2 elements, which the logic
attempted to compile to an ordinary property instead of a method. It's
ambiguous with
{
d: null
}
if () were to compile to null. It currently compiles to nothing (#32),
but the argument stands.
This change hence introduces this syntax
(object (method 'd ()))
similarly to how getters and setters already allow empty bodies with
(object (get 'd ())
(set 'd ()))
I also moved the check for the initial "get" / "set" / "method" atom
above the args.length==2 check, to ensure it takes precedence.
Just in case someone creates a file called "test-all".
|
I understand why you're sending the PR on this branch. I'm on my phone at On Sat, Feb 6, 2016, 17:27 An [email protected] wrote:
|
| @@ -374,7 +374,7 @@ Property access uses the `.` macro. | |||
| If you wish you could just write those as `a.b.c` in eslisp code, use the | |||
| [*eslisp-propertify*][10] user-macro. | |||
|
|
|||
| For *computed* property access, omit the leading colon. | |||
There was a problem hiding this comment.
This is probably better in another PR. It's an oversight independent of this PR, and is better judged separately.
| # This makes error handling neater: the top-level macro function catches | ||
| # this type of Errors and prepends information about which parameter was | ||
| # being processed when it occurred. | ||
| class ObjectParamError extends Error |
There was a problem hiding this comment.
I would also like to mention that this really needs to be done everywhere, not just within the (object) macro. But I'll save that for a later exercise.
Also, please include the stack. It makes for easier debugging. (Code generation serves as a good example here.)
# This is sufficient. Error.captureStackTrace is V8-specific, but nearly every
# implementation has a `stack` instance property on Errors.
if Error.captureStackTrace
Error.captureStackTrace this, ObjectParamError
else
@stack = new Error!stack
|
@anko I left several notes inline where I feel you could pull those out of this PR, and I would be much more likely to merge them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
methodatom to denote method properties (see 98ddff3)It's probably administratively neater to merge this into your
propertybranch so everything about it stays in the same PR and we stay on the same page about what's happening.I still see the issue that
{ set : 5 }can't be expressed, because that(object (set (a)))→({ set [a](b) {} }), but this is progress.