Skip to content
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

finish and replace on ClassDescriptor aren't currently used in ToClassDescriptor #220

Closed
tjcrowder opened this issue Jan 18, 2019 · 2 comments

Comments

@tjcrowder
Copy link
Contributor

ClassDescriptor has finish and replace properties, but ToClassDescriptor doesn't do anything with those properties.

I think where we came to in #182 and which was implemented in #199 would make finish and replace on ClassDescriptor obsolete. But I could be mistaken

I think we need to either:

  1. Remove them from ClassDescriptor, or
  2. Modify ToClassDescriptor to have it look for finish and replace properties and, if either exists, add appropriate type="hook" element(s) to a shallow copy of elementsObject just prior to calling ToElementDescriptors.

(1) is the simple solution; if you want hooks, add them to the elements array. (2) is more concise to use.

FWIW: The direction I lean varies by the minute. On the one hand, KISS. On the other hand, it's pretty simple to say "putting a finish/replace function on the class descriptor effectively adds a type="hook" element with a finish/replace function to elements".

Happy to add it to my PR queue once a decision is made.

@nicolo-ribaudo
Copy link
Member

👍 for option 1

@littledan
Copy link
Member

littledan commented Jan 20, 2019

Let's read the properties and throw if they are present, and remove them from ClassDescriptor as you say. Option 2 is a bit redundant, as you explained.

tjcrowder added a commit to tjcrowder/proposal-decorators that referenced this issue Jan 20, 2019
…dd proactive checks that throw for them in ToClassDescriptor.
littledan pushed a commit that referenced this issue Jan 20, 2019
…roactive checks that throw for them in ToClassDescriptor. (#222)
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

No branches or pull requests

3 participants