-
Notifications
You must be signed in to change notification settings - Fork 73
Allow attributes to be used on types, variables, and table fields #147
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
base: master
Are you sure you want to change the base?
Conversation
|
Didn't bother adding ebnf for each bit of syntax where attributes will now be allowed, as its the same as how function attributes are applied except adjusted for each bit of syntax this RFC adds attributes to. I think tables might need some ebnf defined (because of how attributes are merged)? Although unsure how to do that, so if someone wants to add that in it'd be appreciated. |
|
Is there a difference between these? local on_type: @deprecated T
@deprecated
local on_variableSeems like the answer is yes? For example: type deprecate<T> = @deprecated T
@deprecated
type deprecated<T> = T
local use_deprecate: deprecate<number> = 67 -- This is fine
local use_deprecated: deprecated<number> = 67 -- This is not fine, you're using a deprecated type
print(use_deprecate) -- This is not fine, you're using a deprecated variableI think this is something you need to specify in the RFC. I feel like this will require a major redesign on how the Also, what about attributes on loops? Something like an |
|
I think instead of adding attribute syntax to virtually every language construct in one fell swoop, we should leave grammar incorporation to separate RFCs that actually introduce useable attributes for a specific construct, e.g. an This way, we won't accidently introduce attribute syntax in places where we'll never end up supporting any attributes, and discussion about semantics like merging deprecation messages can be specialized to the attribute being considered. In this RFC's case, I think it should just focus on adding |
Will get to rewriting this RFC eventually to address this, I just haven't made time yet.
Maybe, I'd have to think about it more but I think that'd be better off in its own RFC as it doesn't effect any current attributes. And also would require this RFC to explain the usefulness of having attributes on for loops, and at that point the RFC is basically adding
Before Also there are type specific attributes that I think would be good, like a type BaseCat = {
name: string
}
type FeralCat = BaseCat & {
colony: string
}Which when used on FeralCat will make it appear as this on hover: { name: string, colony: string }Instead of how it appears currently: { name: string } & { colony: string }This example is simple but this does get annoying with larger tables, and an actual RFC for such an attribute would have a better example. I just don't want to include this in the RFC as at that point I might as well have this RFC add this attribute, which then makes this RFC longer than it needs to be. |
|
Please remove the word 'everywhere' and define exactly the specific places where you want to propose having attributes. We are not going to accept an RFC for attributes everywhere. |
I think the best way here is to just disallow this? As doing this instead would also be fine: @deprecated
local use_deprecate: number = 67
print(use_deprecate) -- not fineWhich the RFC already disallows, but the wording isn't that great. The closest the RFC allows is the following, but it has a different behavior: @deprecated
type AnimalType = "Cat" | "Dog"
-- this type will lint because it uses a deprecated type
type Animal = {
type: AnimalType
} |
| Although this will cause the return in the module to be linted, but this lint won't be passed on to consumers of the module: | ||
|
|
||
| Module: | ||
|
|
||
| ```luau | ||
| -- DeprecatedApi: Variable 'puppy' is deprecated, use 'dog' instead Luau(22) | ||
| return table.freeze({ | ||
| puppy = puppy, | ||
| dog = "woof", | ||
| }) | ||
| ``` | ||
|
|
||
| Consumer: | ||
|
|
||
| ```luau | ||
| -- No lint occurs on the import | ||
| local module = require("@module") | ||
|
|
||
| -- DeprecatedApi: Member 'puppy' is deprecated, use 'dog' instead Luau(22) | ||
| print(module.puppy) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will probably end up not having it work like this, as this is just sorta weird behavior?
But variables I would like to keep, as I could imagine it being useful for those who were not doing best practices before.
Like in the case of someone writing their entire game in one script, where they want to deprecate a variable because they split out its logic to a different module. With find and replace being time consuming when they're likely to just be rewriting most of everything. So having the @deprecated attribute on the variable would work as a reminder to not use that variable as they refactor.
But this is literally only good for this one somewhat niche usecase, so perhaps ditching variable support would be a good idea?
Rendered