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

chore: remove method that's same as base method #1445

Merged

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Nov 22, 2023

If you take a look at BaseInvocationScope.call, you'll see that it's essentially the same implementation as this removed method that was overriding it.

@nedsalk nedsalk marked this pull request as ready for review November 22, 2023 14:34
@nedsalk nedsalk self-assigned this Nov 22, 2023
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
86.69% (+0.07% 🔼)
6063/6994
🟡 Branches 70.7% 941/1331
🟡 Functions
77.41% (+0.06% 🔼)
997/1288
🟢 Lines
86.67% (+0.08% 🔼)
5806/6699

Test suite run success

1477 tests passing in 259 suites.

Report generated by 🧪jest coverage report action from 54277ae

@nedsalk nedsalk enabled auto-merge (squash) November 22, 2023 14:44
Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

Seems fine since the tests pass, but how is the function scope still working as it does when used on the Script version?

This was part of the refactor to a new file long ago in #745 and at the time these callsites matched.

It does look safer to remove the copy, as they have since branched with slightly different implementation details.

@nedsalk nedsalk marked this pull request as draft November 29, 2023 12:38
auto-merge was automatically disabled November 29, 2023 12:38

Pull request was converted to draft

@arboleya arboleya added the chore Issue is a chore label Dec 8, 2023
@nedsalk nedsalk marked this pull request as ready for review December 12, 2023 13:47
@nedsalk nedsalk enabled auto-merge (squash) December 12, 2023 13:50
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Should we merge this after the beta-5 release, just in case?

@nedsalk nedsalk marked this pull request as draft December 12, 2023 15:33
auto-merge was automatically disabled December 12, 2023 15:33

Pull request was converted to draft

@nedsalk nedsalk changed the base branch from master to rc/salamander December 13, 2023 08:47
@nedsalk nedsalk added this to the Salamander milestone Dec 13, 2023
@nedsalk nedsalk marked this pull request as ready for review December 13, 2023 08:48
@nedsalk
Copy link
Contributor Author

nedsalk commented Dec 13, 2023

Moved base from master to rc/salamander and reopened for reviews.

@nedsalk nedsalk changed the title refactor: remove method that's same as base method chore: remove method that's same as base method Dec 13, 2023
@nedsalk nedsalk requested a review from arboleya December 13, 2023 15:59
@nedsalk nedsalk enabled auto-merge (squash) December 14, 2023 08:34
@nedsalk nedsalk merged commit 5d5c939 into rc/salamander Dec 14, 2023
8 checks passed
@nedsalk nedsalk deleted the ns/refactor/-remove-call-method-on-ScriptInvocationScope branch December 14, 2023 18:07
@nedsalk nedsalk mentioned this pull request Dec 14, 2023
44 tasks
arboleya added a commit that referenced this pull request Jan 26, 2024
* chore: updating code owners (#1496)
* docs: purge hardcoded snippets on 'using typegen' page (#1403)
* chore: remove method that's same as base method (#1445)
* chore: implement RC workflow (#1497)
* Revert "feat: add `Predicate.getTransferTxId` helper (#1467)"
* chore: fix rc release string replace (#1529)
* docs: Update some hyperlinks to reference the new documentation hub (#1520)
* chore: improve rc release message (#1559)
* feat: GraphQL subscriptions (#1374)
* chore: pin `graphql-request` to `v5` (#1567)
* chore: upgrade `tsx` (#1574)
* feat: migrate from Jest to Vitest (#1310)
* chore: fix temp test workflow (#1579)
* chore: update required node engine in `create-fuels` (#1582)
* chore: add node version test matrix (#1575)
* chore: fix broken rc message (#1580)
* chore: update nodejs to v20 (#1544)
* feat: accepting addresses as `string` (#1583)
* chore: properly format the PR coverage report comment (#1586)
* fix: flaky test (#1590)
* docs: update `deposit-and-withdraw` page (#1591)
* feat: retry mechanism (#1474)
* feat: replaced `semver` dependency with custom implementation (#1594)
* feat: replace `elliptic` with `@noble/curves` (#1601)
* chore: fix CI failing due to missing tag in test (#1614)
* feat: improve ABI Coders `decode` validation (#1426)
* fix: do not generate a coverage diff without coverage artifact (#1629)
* chore: pinpoint vitest to 1.0.4 (#1637)
* chore: remove `ethers` dependency from `utils` (#1640)
* fix: `getOperation` for `Transfer Asset` (#1619)
* fix: remove external font dependencies (#1642)
* fix: generate RC PR comment on `pull_request` event only (#1648)
* fix: fix failing `rc` comment (#1657)
* chore: add missing test group (#1658)
* feat: implement browser compatibility testing (#1630)
* chore: fix string replace in `rc` ci (#1659)
* chore: adding extra reporters (#1661)
* chore: manually trigger `rc` CI (#1660)
* feat: use `submitAndAwait` graphql endpoint (#1615)
* fix: flaky retry test (#1654)
* feat: create a wallet without a provider (#1566)
* chore!: Share single chainConfig and review node-related utilities (#1602)
* chore: use new temporary coverage artifact (#1676)
* fix: internalizing `findBinPath` utility (#1679)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants