-
Notifications
You must be signed in to change notification settings - Fork 9
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
style: add warn for shadow variables (variables in scope that share s… #663
Conversation
I wasn't involved in conversation about this, but thought I would give it a try. (https://energyweb.atlassian.net/jira/software/projects/SWTCH/boards/54?selectedIssue=SWTCH-1437) |
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.
This rule isn't specific to iam-lib. Would it make sense to add it to shared config https://github.com/energywebfoundation/shared-configs/tree/master/packages/eslint-config?
Yes, but let's discuss if it is something we want first. Maybe we could fix some of the warnings this introduces in this PR (e.g. just in |
@whitneypurdum would you mind fixing a handful of the new shadow warnings in this PR so that we can see what it looks like please 🙇♂️? No need to fix them all to start, but I think we should fix them before we merge this as we don't want to introduce a change which then leads to people seeing warnings when they are development. IMO, that isn't very pleasant dev experience. |
6db96eb
to
66fea73
Compare
@jrhender I fixed a few, there are 19 no-shadow warnings split between e2e and non-e2e files. What is the goal of this PR - to fix all instances of no-shadow? Or just in non e2e? |
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.
I fixed a few, there are 19 no-shadow warnings split between e2e and non-e2e files. What is the goal of this PR - to fix all instances of no-shadow? Or just in non e2e?
IMO, if we are enabling the rule, then we should resolve all warnings that result from it (as you've done, thank you)
I think there is just one more to fix on line 1062 of claims.service.e2e.ts
and then this is good to merge, IMO
## [7.0.0-alpha.4](v7.0.0-alpha.3...v7.0.0-alpha.4) (2022-10-18) ### Style changes * add warn for shadow variables (variables in scope that share s… ([#663](#663)) ([dd28b49](dd28b49))
🎉 This PR is included in version 7.0.0-alpha.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [7.0.0](v6.2.0...v7.0.0) (2022-12-06) ### ⚠ BREAKING CHANGES * **claims:** Payload of selfsigned claim is arbitrary object ### Bug Fixes * bump ew-credentials to 2.2.1-alpha.296.0 ([#662](#662)) ([9af689c](9af689c)) * **claims:** change selfsigned claim to record ([9986876](9986876)) * commit for 2c0a860 ([45cdac0](45cdac0)) * publish assets on-chain ([#654](#654)) ([2c0a860](2c0a860)) * verify credential before issuer verification ([111923b](111923b)) ### Refactoring * **claims:** merge ClaimData with vc-verification ([0e1fafa](0e1fafa)) ### Documentation * add asset credential registration to documentation ([ba6f7ba](ba6f7ba)) ### Style changes * add warn for shadow variables (variables in scope that share s… ([#663](#663)) ([dd28b49](dd28b49))
🎉 This PR is included in version 7.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…ame name)
Description
I wasn't involved in conversation about this, but thought I would give it a try. (https://energyweb.atlassian.net/jira/software/projects/SWTCH/boards/54?selectedIssue=SWTCH-1437)
Extends eslint.rc to add warn for shadow variables https://eslint.org/docs/latest/rules/no-shadow
You can see some no-shadow warnings here https://github.com/energywebfoundation/iam-client-lib/pull/663/files, is this what you were intending to achieve with this rule?
Contributor checklist