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

Added "Readonly" flag for readonly properties #623

Closed
wants to merge 8 commits into from

Conversation

rklfss
Copy link
Contributor

@rklfss rklfss commented Oct 13, 2017

Readonly reflection flag for readonly properties is added.

This PR is related to #611

The way, the abstract flag is added to the reflection is changed. It fixes the issue that abstract accessors weren't considered so far.

To extend the discusson on #611:
Does it make sense to remove the Const flag in favor of the Readonly flag?

@rklfss
Copy link
Contributor Author

rklfss commented Oct 13, 2017

Const flag is added to const enumerations.

@aciccarello
Copy link
Collaborator

aciccarello commented Oct 13, 2017

Thanks for the PR. This would close #585 assuming it is also added to the default themes

@shlomiassaf
Copy link
Contributor

@makana
TypeScript has a clear separation between Const and Readonly as so it is better that we follow their logic. When we are aligned with the language it's easier to understand the code and less chance of getting surprised.

When creating the UI it's easier as well, you want to show readonly on classes and const on variables and if they are consolidated you have to do some logic which is not optimal.

It's better to stick to the same path as TS and not create local terminology...

@rklfss
Copy link
Contributor Author

rklfss commented Oct 16, 2017

@shlomiassaf
Agreed. At least with my last commit (add const flag to enumerations) removing the const flag option is obsolete.

@aciccarello
Since the flag is added to the "relevantFlags" array, it will be automatically rendered in the default themes.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 28, 2020

#1268 made these changes and has been merged in.

@Gerrit0 Gerrit0 closed this Jun 28, 2020
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.

4 participants