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

infra(unicorn): prefer-negative-index #2512

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 27, 2023

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: infra Changes to our infrastructure or project setup labels Oct 27, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 27, 2023
@ST-DDT ST-DDT requested review from a team October 27, 2023 22:23
@ST-DDT ST-DDT self-assigned this Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #2512 (390432d) into next (48a7af4) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2512   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2820     2820           
  Lines      255037   255037           
  Branches     1082     1082           
=======================================
+ Hits       253992   253998    +6     
+ Misses       1017     1011    -6     
  Partials       28       28           

see 1 file with indirect coverage changes

@xDivisionByZerox
Copy link
Member

Personally, I find negative indexes quite unintuitive. Usually, a negative index is used to purposefully present an out of bound index. Look at Array.prototype.indexOf: an index of -1 is returned in case the element was not found. Following the negative index logic, the element is actually positioned at array.length - 1, which is not the case.

The rule itself sadly doesn't have any description on WHY you should use it. I'm welcome for anybody providing reasoning on this.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Oct 30, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 31, 2023

Team Decision

  • We accept this.
  • Although we might want a code rewrite for the method that is currently affected.
    • e.g. change splice remove + index access -> index access adjusted by removed elements.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Oct 31, 2023
@ST-DDT ST-DDT enabled auto-merge (squash) November 6, 2023 08:34
@ST-DDT ST-DDT merged commit 813a34d into next Nov 6, 2023
20 checks passed
@ST-DDT ST-DDT deleted the infra/unicorn/prefer-negative-index branch November 6, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants