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

Interface should start with I code guideline #624

Closed
hexa00 opened this issue Oct 12, 2017 · 14 comments
Closed

Interface should start with I code guideline #624

hexa00 opened this issue Oct 12, 2017 · 14 comments

Comments

@hexa00
Copy link

hexa00 commented Oct 12, 2017

See: https://github.com/theia-ide/theia/wiki/Coding-Guidelines Intefaces section

This is from #608 and #623

Any objections ?

@akosyakov
Copy link
Member

I did not mean that interfaces should start with I with #623, but that if they don't start they should be treated as JSON object interfaces.

@svenefftinge
Copy link
Contributor

Please note that in TS any class declaration is also an interface declaration.

@hexa00
Copy link
Author

hexa00 commented Oct 12, 2017

I did not mean that interfaces should start with I with #623, but that if they don't start they should be treated as JSON object interfaces.

So what I've put in the coding guidelines is correct... Real interfaces with I and JSON objects interfaces without them.

You would add that a JSON object interface could still have an I ?

@hexa00
Copy link
Author

hexa00 commented Oct 12, 2017

Please note that in TS any class declaration is also an interface declaration.

Yes but we should not use a a class as an interface.

@akosyakov
Copy link
Member

akosyakov commented Oct 12, 2017

@hexa00 The most part of the code base is written in line with #624 (comment). Introducing this rule will make most of the code invalid for no reason.

Yes but we should not use a a class as an interface.

One can do, for example, to provide an interface and stub implementation, look at QuickOpenService.

@svenefftinge
Copy link
Contributor

Yes but we should not use a a class as an interface.

All references besides the constructor (and instanceof expression) are in fact referencing the interface not the class. I think it is important to really understand TypeScript and then we should properly use it. If there are certain features that do harm I'm fine with having guidelines to disallow them. But if we just feel uncomfortable because it was different in another language, we need to learn TS and get over it.

@hexa00
Copy link
Author

hexa00 commented Oct 13, 2017

One can do, for example, to provide an interface and stub implementation, look at QuickOpenService.

@akosyakov What is the point of that ? compared to just an interface ? it even says in the doc
* It should be implemented by an extension, e.g. by the monaco extension.

And the stub is empty.

I think it is important to really understand TypeScript and then we should properly use it

This is what we are trying to do.

My current understanding is that even if the underlying javascript implementation of TypeScript means that basically interfaces do not exist. And interfaces get translated into classes when used that doesn't mean that using a class as interface is idiomatic to TypeScript.

The way I see it TypeScript has interfaces so we should use interfaces when we want interfaces no ?

@akosyakov
Copy link
Member

What is the point of that ?

Provide a default implementation which does nothing. In the case, if the monaco extension is not present or one doesn't want to use the quick open.

@hexa00
Copy link
Author

hexa00 commented Oct 13, 2017

Provide a default implementation which does nothing. In the case, if the monaco extension is not present or one doesn't want to use the quick open.

OK that's fine.

Just to recap:

  • Much of the current code uses classes as interfaces and doesn't use I for these.
  • JSON-Object representations interfaces are not using I
  • Some of the code uses I for pure interfaces.

So options:

Don't use I:

  • To have a class of the same name you should use the class as the interface.
  • Removes clarity about is this an API and what is implementation.

Use I:

  • More clear API wise, otherwise I don't see the point if we're doing to allow classes as interfaces.

I think given all this the main point is if we're not going to use I we have to be clear that the base class is API and not confuse it with the implementation.

WDYT ? Ideas on how to make this clear ?

@akosyakov
Copy link
Member

Removes clarity about is this an API and what is implementation.

I don't think using I is going to clarify what is API. API can consist of any lang elements including classes, as well as an internal implementation can use interfaces.

Otherwise, we will write tons of boilerplate to define and bind services, e.g. IFrontendApplication and then provide a single implementation FrontendApplication plus IFrontendApplication symbol and 3 lines of bindings to glue all together.

To define public APIs we will need to use other means like @internal jsdoc tag, similar to other TS projects, or something else, like a custom decorator.

WDYT ? Ideas on how to make this clear ?

One can always add Impl suffix to an implementation to be explicit.

@hexa00
Copy link
Author

hexa00 commented Oct 13, 2017

I don't think using I is going to clarify what is API. API can consist of any lang elements including classes, as well as an internal implementation can use interfaces.

True what I just have said it makes it less clear what is an Interface...

And I like the decorators idea.

What I'm afraid of however is someone adding code to let's say the QuickOpener class and not realizing that by doing so he's breaking other code that implements that.

Especially if external extensions depend on that as API. But I guess the custom decorator will here there too.

I'm ok with Impl.

So should we make it so that we do not use I and use Impl if need be ?

So like for Logger you would get interface Logger impl: LoggerImpl

@svenefftinge
Copy link
Contributor

So should we make it so that we do not use I and use Impl if need be ?

+1

@hexa00
Copy link
Author

hexa00 commented Oct 20, 2017

OK I've updated the coding guideline with this using Impl instead of I.

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