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

Replace strip! by strip on compute_digest method #650

Merged
merged 1 commit into from
Jan 2, 2023
Merged

Conversation

pitbulk
Copy link
Collaborator

@pitbulk pitbulk commented Jan 2, 2023

This PR replaces #647, Issue reported at #603

@bramleyjl This PR contains only 1 commit vs the one you created with 6 commits.

This PR is for a fix to the compute_digest method that was rendering a nil digest after Base64-encoding it. The reason for this bug is the usage of strip!, a method that returns nil if no whitespace is found on either end of the string to be stripped. By replacing it with strip, an identical method that will return the original string if no whitespace is found to be stripped, this bug can be prevented.

@RDeckard
Copy link

RDeckard commented Jan 5, 2023

Indeed, as lots of Ruby bang methods, #strip! returns nil if it didn't make any change to its receiver. 🎯

But those methods are also known to be more efficient (especially with big objects), as they mutate their receivers instead of duplicating it.

So if you want the best of both worlds (knowing that here mutation didn't generate any problem by itself), you could also have done Base64.encode64(digest).tap(&:strip!).
You probably already know that, but the short explanation is that the #tap method allows us to do whatever we want with its receiver in its block, then returns the receiver itself (i.e. not what the block returns). Therefore here, the receiver will be mutated by #strip! in the block, then #tap will return this mutated receiver itself instead of the #strip! method result (which could be nil indeed). 🙂

Nevertheless I am not familiar with this gem, so I have no idea if it would have had any impact here.

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.

None yet

2 participants