Skip to content

Conversation

@ChefBingbong
Copy link

@ChefBingbong ChefBingbong commented Jun 18, 2025

Motivation
This PR takes care of a pending task, which is to re-enable the biome noParamaterReAssign style lint and to fix all instances in the codebase where this rule is breached

changes

  1. Re-enabled biome's noParamaterReAssign
  2. fixed all places where this rule was breached,
  3. ensured the lint command in root runs with no warnings
  4. ensured all test suites remained unaffected by the relevant code changes

@ChefBingbong ChefBingbong requested a review from a team as a code owner June 18, 2025 14:39
@CLAassistant
Copy link

CLAassistant commented Jun 18, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

Please make sure to reflect this change only for functions where parameters are really reassigned within function.

}

setUintBigint(uintBytes: number, offsetBytes: number, valueBN: bigint): void {
setUintBigint(uintBytes: number, offsetBytes: number, _valueBN: bigint): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see valueBN is been reassigned anywhere in this function. Also primitives as parameters is not an issue to be assigned anyway.

Copy link
Author

@ChefBingbong ChefBingbong Jun 18, 2025

Choose a reason for hiding this comment

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

Hi there @nazarhussain . thanks for your comment. Actually it is being reassigned here

https://github.com/ChefBingbong/ssz/blob/53591ed4ca031dee9f1305c775eab4096675568c/packages/persistent-merkle-tree/src/node.ts#L289

in the last else of the function.

also this pr tackles this issue. so this is the motivation behind these changes

ssz/biome.jsonc

Lines 19 to 20 in 1a34fe8

// TODO: There are two many usages, will fix in separate PR
"noParameterAssign": "off",

re-enabling this biome flag requires these locations (i.e this prs changes) to be fixed. however i made sure that the the changes are compatabile with the current test suiting and dont change expected code functionality

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

The changes related to primitives should be removed. All values of number, bigint or any other primitive is passed by value, so no issue if these are reassigned in local function scope.

We should focus the changes only object or complex types where accidental parameter change have any side effect.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants