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

[Feat] Adding the support for setters #94

Closed
wants to merge 1 commit into from

Conversation

JulesGuesnon
Copy link
Contributor

Description

This PR tries to handles #39
It's my first time touching to checker, so not sure if what I'm doing is exactly what is expected, so feel free to tell me if I'm mistaking somewhere

todo!()
PropertyValue::Getter(_) => return Err(SetPropertyError::NotWriteable),
PropertyValue::Setter(setter) => {
// Setters must have 1 parameters, so it's supposed to be catch at parsing time
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I had a thought to make it fixed in the parser with generics. But the additional complexity outweighed the benefit so it is just assumed!

@kaleidawave
Copy link
Owner

This is a great implementation of TypeScript's behaviour for setters! Un/fortunately with Ezno we can do one better and actually evaluate the setter's side effects. Because other Ezno features rely of this behaviour existing it means that ALL side effects that can be observed, need to be.

It is my fault, I was a bit rushed in June and should have added more information to the issue. I have updated the message in #39 to describe the way to do in accordance with Ezno's type system. It reuses the calling logic which does the same parameter restriction checking in this first commit.
Given the addition of CallingInput in #91 I think you should be confident with using those functions! Hopefully that clears it up.


Might seem a bit unnecessary, but things like array.length = 2 has the effect of deleting all number properties after 2 (need to double check that). So it can come in handy for catching a bunch of things TSC doesn't not just in user code, but in places that use platform APIs!

@kaleidawave
Copy link
Owner

Hey @JulesGuesnon, apologies that there wasn't information on the issue prior to the PR. Let me know if you still want to work on this based on the new information in the issue, or want any more pointers or me to start the implementation off etc.

@JulesGuesnon
Copy link
Contributor Author

hey @kaleidawave really sorry for the delay, the end of this year has been a lot of changes in my personal life, so I've suddenly became quite busy.
I still want to work on this, but not sure when I'll have the time tho. So if you want to do it or have the time to do so, please feel free to do it, otherwise I'll do it once I find some time

@kaleidawave
Copy link
Owner

Awesome, totally understand 👍 Closing this PR as I think it would be better for any second attempt to start afresh. Thanks for the attempt (and thanks for #91, have noticed the codebase feeling a lot less messy recently)

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

Successfully merging this pull request may close these issues.

2 participants