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

Implement Abstract, Const and Let flags #611

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

shlomiassaf
Copy link
Contributor

This PR add the following enums to ReflectionFlag:

  • Abstract
  • Const
  • Let

Additionally, each enum has it's implemented converter logic added.

Why?

No need to explain why Abstract symbols should reflect in the UI.
For Let and Const, these are also important, Const is a read only symbol so the user should be aware of that. Let does not have much effect since we expose structure and not implementation hence let and var in that context are the same, however, if we state const we should state let as it's a language feature and should reflect in the UI to be consistent.

Abstract

In ClassConverter added logic to set Abstract flag to each DeclarationReflection that represents an abstract class, following the same logic for each abstract member of that class (FunctionConverter, VariableConverter)

Theming Notes:

  • The theme's does not require change for members as the Abstract enum is also set as a relevant flags, hence showing in native themes.
  • The overwrites property of a member the overwrites an abstract parent member is left as is. This means it will still show overwrites which is not the right terminology, might require a custom fix in the templates. You can see if you "overwrite" an abstract parent by checking if it has the Abstract flag.
  • Abstract classes have no UI representation as it does not exist in the theme's, need to implement in the themes.

Const & Let

In VariableConverter added logic to set Const or Let flag to each DeclarationReflection that represents a variable where the typescript node's parent has Const or Let node flags set.
A var variable should be inferred whenever a reflection of kind ReflectionKind.Variable has no Let or Const flags.

Theming Notes:

  • The theme's does not require change because Const and Let enums are also set as a relevant flags, hence showing in native themes. (var will not display, as is now)

add ReflectionFlag.Const and support for variable
add ReflectionFlag.Let and support for variable
@shlomiassaf
Copy link
Contributor Author

image

image

Copy link
Collaborator

@aciccarello aciccarello left a comment

Choose a reason for hiding this comment

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

Great job on this. I'm not against merging as is but I'm wondering if let should be invisible in the docs like var is. It seems like an implementation detail that is irrelevant to the user. If we are aiming for consistency, I'd imagine that let should be consistent with var rather than with const.

@shlomiassaf
Copy link
Contributor Author

@aciccarello There's no harm in being explicit here...

Consider a namespace export with nested namespace exports.
var and let has logical meaning in such scenario.

Of course rare scenario :) but it might be.

Anyway, we should let the theme decide whether to display it or not... by default it should be on.

IMHO, it should always be clear, var let or const...

@aciccarello
Copy link
Collaborator

Makes sense to me. Thanks for your work!

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