Skip to content

Conversation

@crdev13
Copy link
Contributor

@crdev13 crdev13 commented Aug 21, 2023

Fixes #4454
#minor

Description

This small PR just changes an argument type in the static validate method of the EndorsementsValidator class.

Specific Changes

  • Before this change the validate method was receiving an array for endorsements, now it now receives a HashSet and reduces the time comlexity when it tries to verify if a key(channelId) exists.

Testing

All unit tests passed
endorsements_validator

@crdev13 crdev13 requested a review from a team as a code owner August 21, 2023 03:52
@crdev13
Copy link
Contributor Author

crdev13 commented Aug 21, 2023

@microsoft-github-policy-service agree

* @returns {boolean} True is the channelId is found in the Endorsement set. False if the channelId is not found.
*/
static validate(channelId: string, endorsements: string[]): boolean {
static validate(channelId: string, endorsements: Set<string>): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the function's signature would break the backward compat. Instead, could you create a new function that receives the Set argument and call it inside the old validate function?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to breaking this if having another method is clutter. Is there harm in having two?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest a private function to be called inside this old one (passing the converted strings[] as set) so those who are using this with an array won't experience a break.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, probably with a comment about it. Future refactor I suppose if we ever do another major release. v5.

@crdev13 crdev13 requested a review from ceciliaavila January 14, 2024 23:02
@coveralls
Copy link

coveralls commented Jan 15, 2024

Pull Request Test Coverage Report for Build 7522268835

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.511%

Totals Coverage Status
Change from base Build 7462894606: 0.0%
Covered Lines: 20408
Relevant Lines: 22872

💛 - Coveralls

@crdev13
Copy link
Contributor Author

crdev13 commented Feb 26, 2024

@ceciliaavila When you get a chance can you please run pr-style.yml / pr-style (pull_request) again?

@tracyboehrer tracyboehrer merged commit 533162c into microsoft:main Feb 27, 2024
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.

Use HashSet instead of string array for endorsement

4 participants