-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Added _triggerConfigChange #6025
Conversation
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.
Hi, @Parthib314. Thank you for your contribution. This PR looks good to me. Just please add changes to the CHANGELOG.md file
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6025 +/- ##
=======================================
Coverage 85.85% 85.85%
=======================================
Files 156 156
Lines 7425 7425
Branches 2018 2018
=======================================
Hits 6375 6375
Misses 1050 1050
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Done. |
I am sorry for the misunderstanding. As you have changes in the Template: [Unreleased]Changed
|
Oops sorry. I think now it's good. |
packages/web3-core/CHANGELOG.md
Outdated
@@ -99,6 +99,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- If a transaction object with a `data` property is passed to `txInputOptionsFormatter`, it will now be replaced with `input` (#5915) | |||
- The types `TransactionTypeParser` and `TransactionBuilder` are now utilizing the type `Transaction` for the transaction object. (#5993) | |||
- no need for polyfilling nodejs `net` and `fs` modules | |||
- Changed directly emitting events to `_triggerConfigChange` in `Web3Config` class (#5813) |
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.
- Changed directly emitting events to `_triggerConfigChange` in `Web3Config` class (#5813) | |
- `Web3Config` now consistently uses `_triggerConfigChange` (#5813) |
Although I don't really think a change to the CHANGELOG.md
is needed since this is an internal change that won't affect users
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.
@Parthib314,
Generally only notable changes are mentioned in changelog that effect end users. IMO this PR doesn't effect end uses so either you may remove your updates in changelog files or conflicts in few files needs to be fixed before we merge .
This PR LGTM, Thanks for your contribution in 4.x.
Otherwise, looks good to me! Thank you for your contribution 🚀 |
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.
Thanks for your contribution.
And as stated, please revert all changes to the CHANGELOG.md files. As they are updated only if there are changes concerning the end-user.
Done 👍👍 |
Updated CHANGELOG.md Updated CHANGELOG.md Signed-off-by: Parthib <parthibdutta02@gmail.com>
Signed-off-by: Parthib <parthibdutta02@gmail.com>
Many thanks @Parthib314
|
But I already signed in my commits. |
When your commits are signed, then you will have the |
Description
Added
_triggerConfigChange
consistently in Web3Config for triggering events.Fixes #5813
Type of change
Checklist for 4.x: