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.

Sign up for GitHub

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

Jump to bottom

Added _triggerConfigChange #6025

Merged
merged 5 commits into from
Apr 25, 2023
Merged

Added _triggerConfigChange #6025

merged 5 commits into from
Apr 25, 2023

Conversation

ptdatta
Copy link
Contributor

@ptdatta ptdatta commented Apr 18, 2023

Description

Added _triggerConfigChange consistently in Web3Config for triggering events.

Fixes #5813

Type of change

Checklist for 4.x:

  • I have selected the correct 4.x base branch.
  • Within the description, the feature or issue is discussed in detail or an issue is linked.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas.

@ptdatta ptdatta mentioned this pull request Apr 18, 2023
Closed
avkos
avkos requested changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@avkos avkos left a 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
Copy link

codecov bot commented Apr 18, 2023 •
edited
Loading

Codecov Report

Merging #6025 (aca1f54) into 4.x (5153961) will not change coverage.
The diff coverage is 100.00%.

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
Flag Coverage Δ
UnitTests 85.85% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
web3 ∅ <ø> (∅)
web3-core ∅ <ø> (∅)
web3-errors ∅ <ø> (∅)
web3-eth ∅ <ø> (∅)
web3-eth-abi ∅ <ø> (∅)
web3-eth-accounts ∅ <ø> (∅)
web3-eth-contract ∅ <ø> (∅)
web3-eth-ens ∅ <ø> (∅)
web3-eth-iban ∅ <ø> (∅)
web3-eth-personal ∅ <ø> (∅)
web3-net ∅ <ø> (∅)
web3-providers-http ∅ <ø> (∅)
web3-providers-ipc ∅ <ø> (∅)
web3-providers-ws ∅ <ø> (∅)
web3-rpc-methods ∅ <ø> (∅)
web3-utils ∅ <ø> (∅)
web3-validator ∅ <ø> (∅)

@ptdatta
Copy link
Contributor Author

ptdatta commented Apr 19, 2023

Hi, @Parthib314. Thank you for your contribution. This PR looks good to me. Just please add changes to the CHANGELOG.md file

Done.

@avkos
Copy link
Contributor

avkos commented Apr 19, 2023

I am sorry for the misunderstanding. As you have changes in the web3-core package, please add your comment in the CHANGELOG.md file inside the web3-core package under Unreleased => Changed. And undo changes in the root folder.

Template:

[Unreleased]

Changed

@ptdatta
Copy link
Contributor Author

ptdatta commented Apr 19, 2023 •
edited
Loading

I am sorry for the misunderstanding. As you have changes in the web3-core package, please add your comment in the CHANGELOG.md file inside the web3-core package under Unreleased => Changed. And undo changes in the root folder.

Template:

[Unreleased]

Changed

Oops sorry. I think now it's good.

@avkos avkos self-requested a review April 19, 2023 19:32
spacesailor24
spacesailor24 reviewed Apr 20, 2023
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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

Copy link
Contributor

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.

@spacesailor24
Copy link
Contributor

Otherwise, looks good to me! Thank you for your contribution 🚀

Muhammad-Altabba
Muhammad-Altabba approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@Muhammad-Altabba Muhammad-Altabba left a 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.

@ptdatta
Copy link
Contributor Author

ptdatta commented Apr 24, 2023

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 👍👍

@ptdatta
Added _triggerConfigChange
ed1636a 
Updated CHANGELOG.md

Updated CHANGELOG.md

Signed-off-by: Parthib <parthibdutta02@gmail.com>
@ptdatta
Updated CHANGELOG.md
7fa5597 
Signed-off-by: Parthib <parthibdutta02@gmail.com>
@Muhammad-Altabba
Copy link
Contributor

Many thanks @Parthib314
And your commits must be signed. As stated by GitHub:

Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits.

@ptdatta
Copy link
Contributor Author

ptdatta commented Apr 25, 2023 •
edited
Loading

Many thanks @Parthib314 And your commits must be signed. As stated by GitHub:

Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits.

But I already signed in my commits.

@Muhammad-Altabba
Copy link
Contributor

Many thanks @Parthib314 And your commits must be signed. As stated by GitHub:

Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits.

But I already signed in my commits.

When your commits are signed, then you will have the Verified tag. See how the other commits have Verified tag:
image

@spacesailor24 spacesailor24 merged commit 7c13a90 into web3:4.x Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avkos avkos avkos approved these changes

@nikoulai nikoulai nikoulai approved these changes

@spacesailor24 spacesailor24 spacesailor24 approved these changes

@Muhammad-Altabba Muhammad-Altabba Muhammad-Altabba approved these changes

@boy520520456 boy520520456 boy520520456 approved these changes

@jdevcs jdevcs Awaiting requested review from jdevcs jdevcs is a code owner

@luu-alex luu-alex Awaiting requested review from luu-alex luu-alex is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ptdatta @avkos @spacesailor24 @Muhammad-Altabba @nikoulai @jdevcs @boy520520456