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

Review PR: Abstract classes #2946

Closed
wants to merge 6 commits into from

Conversation

DickvdBrink
Copy link
Contributor

This PR is only for review (to check if this is the right way to go)

NOTE The baseline isn't correct yet (it says undefined)

If this isn't the right way, feel free to close this right away.
As mentioned in this comment abstract will only be implemented for classes and not for methods/properties

edit
Refs #6, As mentioned there I should have shared a design first but I already created this part a few weeks back (just rewrote on top of the new master branch)

@@ -128,6 +128,7 @@ module ts {
PrivateKeyword,
ProtectedKeyword,
PublicKeyword,
AbstractKeyword,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a future reserved word in ES6, should move to the contextual keywords block. you should also add a test for variables named "abstract" with "use strict";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jup will do, didn't wrote a (full) test-suite yet because I wanted to know if the code was a bit in the right places

@mhegazy
Copy link
Contributor

mhegazy commented Apr 28, 2015

you also need to handle checking for the new modifier in cheker.ts in checkGrammarModifiers.

@@ -1447,6 +1447,10 @@
"category": "Error",
"code": 2501
},
"Cannot create an instance of the abstract class '{1}'": {
Copy link
Contributor

Choose a reason for hiding this comment

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

"cannot use new on an abstract class {1}."

@JsonFreeman may have better message though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oke, Will change this (but I will wait for feedback from JsonFreeman)
The message I got now is more or less the same as the one from C#.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1, and not 0 (for the substitution slot)?

I think I like the "Cannot create an instance" message better.

@DickvdBrink
Copy link
Contributor Author

I do have something to work with :) thanks for the reviews.

The only thing I really wanted to know if I was working in the right spots of the TS code (and that seems to be the case).
So not a lot more to discuss, will work on this stuff and I guess reopen/create a new one when in a more final state?

@paulvanbrenk
Copy link
Contributor

Splitting up the parse/typecheck/emit phases in separate PR-s makes it a lot easier to review things, further don't forget to make the required changes in the language service parts, so we get the correct colorization etc. in the various IDEs.

@aozgaa
Copy link
Contributor

aozgaa commented Jun 9, 2015

Hey @DickvdBrink, I'm currently working on abstract classes. Would it be fine if I worked off of your commits (either from this branch or abstract-classes2) since it looks like you've done some good work so far?

@DickvdBrink
Copy link
Contributor Author

@aozgaa yes ofcourse! Sorry for not taking this feature further because I'm really busy with other stuff.
Feel free to take my commits if they are correct and if they aren't feel free to throw them away! :)

@aozgaa
Copy link
Contributor

aozgaa commented Jun 9, 2015

They look like a great start! We need to formalize the semantics a bit, but you've made good progress! Thanks!

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants