Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_semantic): correct the export determination when a variable and an interface had the same name #4468

Merged
merged 9 commits into from
May 16, 2023

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented May 13, 2023

Summary

Closes #4353

Test Plan

cargo test -p rome_js_analyze -- no_unused_variables

Changelog

  • The PR requires a changelog line

Documentation

  • [ ] The PR requires documentation
  • [ ] I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 13, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 8f7bea2
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64624b0fb44d5c0008a481f0

@github-actions github-actions bot added the A-Linter Area: linter label May 13, 2023
@unvalley unvalley marked this pull request as draft May 13, 2023 15:52
@unvalley unvalley changed the title fix(rome_js_semantic): correct the export determination when a class and an interface share the same name fix(rome_js_semantic): correct the export determination when a class or an variable and an interface share the same name May 13, 2023
feat: make value of bindings_by_name vector

feat: export exportable declration

fix: export order

refactor: restore bindings change

test: update snapshot

chore: add comment

chore: fix test
@unvalley unvalley changed the title fix(rome_js_semantic): correct the export determination when a class or an variable and an interface share the same name fix(rome_js_semantic): correct the export determination when a variable and an interface had the same name May 13, 2023
@unvalley unvalley marked this pull request as ready for review May 13, 2023 17:41
@@ -7,6 +7,9 @@
### Editors
### Formatter
### Linter

- Fix an issue that [`noUnusedVariables`](https://docs.rome.tools/lint/rules/nounusedvariables/) rule did not correctly detect exports when a variable and an `interface` had the same name [#4468](https://github.com/rome/tools/pull/4468)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add the #### Other changes heading

}

#[derive(Debug)]
struct DeclaredBinding {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a brief documentation of the struct, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply renamed DeclaredBinding to BindingInfo. And I added some comment to it but the confusion is might be resolved by naming.

name.clone(),
DeclaredBinding {
text_range: declaration_range,
syntax_kind: *parent_syntax_kind,
Copy link
Contributor

Choose a reason for hiding this comment

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

The name syntax_kind is a bit misleading because I thought we were saving the syntax kind of the node, but instead, we are passing the parent syntax kind. Should we change the name and document it a bit why we save the parent syntax kind?

Copy link
Contributor Author

@unvalley unvalley May 14, 2023

Choose a reason for hiding this comment

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

Right. I also updated the property name from syntax_kind to declaration_kind. I think this change can resolve why the property is needed. How do you feel about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

@ematipico ematipico merged commit 9b21fb1 into rome:main May 16, 2023
@unvalley unvalley deleted the fix-4353 branch May 16, 2023 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
3 participants