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

feature: support @deprecated in type constructors #3698

Open
Wilbert-mad opened this issue Oct 13, 2024 · 4 comments
Open

feature: support @deprecated in type constructors #3698

Wilbert-mad opened this issue Oct 13, 2024 · 4 comments
Labels
help wanted Contributions encouraged priority:medium

Comments

@Wilbert-mad
Copy link

Wilbert-mad commented Oct 13, 2024

Currently, the @deprecated tag is only supported with functions and top-level of a type.

pub type GameLobbyActorMessage {
  FetchOpponent(reply_with: Subject(Result(String, Nil)))
  @deprecated("FetchOpponent is preferred.
  `GroupPlayers`'s calculations may return no longer needed data.
  ")
  GroupPlayers(reply_with: Subject(Result(List(List(String)), Nil)))
}
@Wilbert-mad Wilbert-mad changed the title feature: wider support for @deprecated feature: support @deprecated in type constructors Oct 13, 2024
@lpil
Copy link
Member

lpil commented Oct 18, 2024

Sounds reasonable. Thank you.

I think we would only emit for constructing the variant, we would not for pattern matching on it.

It would be an error to have deprecated attributes on all variants.

@lpil lpil added help wanted Contributions encouraged priority:medium labels Oct 18, 2024
@Wilbert-mad
Copy link
Author

It would be an error to have deprecated attributes on all variants.

  1. When this occurs the parser would completely error, or 2) would it be better to give an LSP warning instead of erroring and give a parser warning? On top of that, we could add a code action to deprecate the type as a whole.
pub type Numbers {
  @deprecated("no")
  One
  @deprecated("no")
  Two
}
──── Code action ────
@deprecated("<message>")
pub type Numbers {
  One
  Two
}

pub type Numbers {
  @deprecated("no")
  One
}
//─── Errors out ────
//Can't deprecate all variants of a type. Consider deprecating the type as a whole.
pub type Numbers {
  @deprecated("no")
  One
}
//─── LSP ────
//Can't deprecate all variants of a type. Consider deprecating the type as a whole.
//─── `gleam run` ───
//Warning:
// ... code ..
//Fix: Can't deprecate all variants of a type. Consider deprecating the type as a whole.

@Wilbert-mad
Copy link
Author

When parsing the Constructor's attributes only the deprecated will be allowed, correct?
Lastly:

I think we would only emit for constructing the variant, we would not for pattern matching on it.

Sorry, but I'm unsure what you mean by this.

@lpil
Copy link
Member

lpil commented Oct 18, 2024

It would be better to error during analysis such that the LS can continue to function.

I don't think a code action would be very valuable, we should focus on other code actions instead. Perhaps in the far-future.

It means the warning would not be emitted if you use a deprecated constructor in a pattern. I'm not entirely sure this is a good idea. My reasoning was that you would still need to pattern match on it as it's still possible even if it is deprecated, but then you may miss this use when moving away from the deprecated variant in your code in order to upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions encouraged priority:medium
Projects
None yet
Development

No branches or pull requests

2 participants