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

Rename ReferenceFlag to ReferenceFlags (plural) #4991

Closed
overlookmotel opened this issue Aug 19, 2024 · 5 comments
Closed

Rename ReferenceFlag to ReferenceFlags (plural) #4991

overlookmotel opened this issue Aug 19, 2024 · 5 comments
Labels
A-semantic Area - Semantic

Comments

@overlookmotel
Copy link
Contributor

All the other semantic flags have plural names - ScopeFlags, SymbolFlags, NodeFlags. ReferenceFlag is the outlier.

Rename:

  • ReferenceFlag to ReferenceFlags.
  • Reference::flag field to flags.
  • Reference::flag method to flags.
  • Reference::flag_mut method to flags_mut.
  • All vars called flag to flags.

Only reason I'm opening an issue for this rather than just submitting a PR is it'd cause merge conflicts with PR stack #4973, so will wait for that to get merged first.

@DonIsaac
Copy link
Contributor

When you make this change, could you add a re-export of ReferenceFlags as ReferenceFlag and mark it as deprecated? This will give downstream consumers time to update their code.

@overlookmotel
Copy link
Contributor Author

Good idea. How do I mark the export as deprecated? (not done that before)

Ditto for the Reference::flag + Reference::flag_mut methods?

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Aug 20, 2024

Actually, on 2nd thoughts, I'm not sure we should get into deprecation for such a simple change. Oxc is not at v1.0.0 yet, and we're making breaking changes all the time. This change is extremely minor compared to most of them, and if we leave an extra flag_mut method doubling the new flags_mut then we'll just have to remember to go and remove it later on (and discuss when it's safe to do that!).

What do you think?

@DonIsaac
Copy link
Contributor

If it's not too much effort, deprecating for a single release could be worth it. It is definitely appreciated when upgrades do not break your code without warning :)

If you think we won't remember to clean it up, then adding a deprecation is not worth it. After all, like you said, it is a very small change.

@overlookmotel
Copy link
Contributor Author

Personally, my view is that it's not worth it. I do understand that breaking changes are always painful, but we are nowhere near a stable API for Semantic, and this change is small beer. If we try to keep deprecated methods etc around for every change to Semantic over next few months I imagine it'll add up to a sizeable workload for us, and for fairly little benefit - people are still going to have to change their code at some point soon.

But also: If we also exported ReferenceFlags and ReferenceFlag, I'm not clear how do you mark ReferenceFlag as deprecated (in a way where people know it's deprecated). If there's no good way of doing that, the deprecation process would seem a bit pointless as no-one will notice the deprecation, and then they'll just experience it as a breaking change later on when we finally do remove the old name.

Boshen pushed a commit that referenced this issue Aug 21, 2024
Boshen pushed a commit that referenced this issue Aug 21, 2024
Boshen pushed a commit that referenced this issue Aug 21, 2024
Boshen pushed a commit that referenced this issue Aug 21, 2024
Boshen pushed a commit that referenced this issue Aug 21, 2024
…sm): rename various `flag` vars to `flags` (#5028)

Part of #4991.
Boshen pushed a commit that referenced this issue Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic
Projects
None yet
Development

No branches or pull requests

2 participants