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

style: add warn for shadow variables (variables in scope that share s… #663

Merged
merged 13 commits into from Oct 18, 2022

Conversation

whitneypurdum
Copy link
Member

@whitneypurdum whitneypurdum commented Oct 11, 2022

…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

  • Breaking changes - check for any existing interfaces changes that are not backward compatible, removed method etc.
  • Documentation - document your code, add comments for method, remember to check if auto generated docs were updated.
  • Tests - add new or updated existed test for changes you made.
  • Migration guide - add migration guide for every breaking change.
  • Configuration correctness - check that any configuration changes are correct ex. default URLs, chain ids, smart contract verification on Volta explorer or EWC explorer.

@whitneypurdum
Copy link
Member Author

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)

@jrhender jrhender requested a review from JGiter October 12, 2022 10:12
Copy link
Collaborator

@JGiter JGiter left a 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?

@jrhender
Copy link
Collaborator

This rule isn't specific to iam-lib. Would it make sense to add it to shared 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 claims.service.ts to start) to see if we like how the fixed code looks?

@jrhender
Copy link
Collaborator

@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.

.eslintrc.js Outdated Show resolved Hide resolved
@whitneypurdum
Copy link
Member Author

@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.

@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?

@whitneypurdum whitneypurdum marked this pull request as draft October 14, 2022 18:40
@whitneypurdum whitneypurdum marked this pull request as ready for review October 17, 2022 13:52
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

@whitneypurdum

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

@whitneypurdum whitneypurdum merged commit dd28b49 into develop Oct 18, 2022
@whitneypurdum whitneypurdum deleted the #SWTCH-1437 branch October 18, 2022 14:44
ewf-devops pushed a commit that referenced this pull request Oct 18, 2022
## [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))
@ewf-devops
Copy link

🎉 This PR is included in version 7.0.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

ewf-devops pushed a commit that referenced this pull request Dec 6, 2022
## [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))
@ewf-devops
Copy link

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants