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

Update contract class to not mutate options object #5394

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

spacesailor24
Copy link
Contributor

closes #5304

@spacesailor24 spacesailor24 added the 1.x 1.0 related issues label Aug 30, 2022
@spacesailor24 spacesailor24 self-assigned this Aug 30, 2022
@render
Copy link

render bot commented Aug 30, 2022

@coveralls
Copy link

coveralls commented Aug 30, 2022

Pull Request Test Coverage Report for Build 2953468494

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.07%) to 74.575%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-helpers/lib/errors.js 1 80.88%
packages/web3-providers-ws/lib/index.js 3 91.81%
packages/web3-eth-contract/lib/index.js 11 91.96%
Totals Coverage Status
Change from base Build 2924335251: -0.07%
Covered Lines: 3272
Relevant Lines: 4136

💛 - Coveralls

@spacesailor24 spacesailor24 marked this pull request as ready for review August 30, 2022 04:09
Copy link
Contributor

@nikoulai nikoulai left a comment

Choose a reason for hiding this comment

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

Nice! The PR solves the problem, but, should this be happening? Is it behavior we want?

@spacesailor24
Copy link
Contributor Author

spacesailor24 commented Aug 30, 2022

Nice! The PR solves the problem, but, should this be happening? Is it behavior we want?

@nikoulai

From my understanding, you should never mutate arguments given to a function (unless that's what the function is specifically designed to do), you should always return the data and let the function caller decide what to do it with it. In the case of _getOrSetDefaultOptions, I think you could argue that mutating the options objects was be design, but it leads to frustrating behavior for the user that isn't straightforward to debug (as mentioned by the user whom opened the issue), so I see this as an improvement for the library whether or not it's actually a bug

This was referenced Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the contract methods call function mutates its options argument
4 participants