Skip to content

Removed Random::Formatter - #34

Merged
hsbt merged 5 commits into
masterfrom
removed-random-formatter
Dec 2, 2024
Merged

Removed Random::Formatter#34
hsbt merged 5 commits into
masterfrom
removed-random-formatter

Conversation

@hsbt

@hsbthsbt commented Dec 2, 2024

Copy link
Copy Markdown
Member

This is another try with #29

@hsbt
hsbtforce-pushed the removed-random-formatter branch from ab4083a to 2fdde4fCompareDecember 2, 2024 03:57
@hsbt
hsbtforce-pushed the removed-random-formatter branch from 2fdde4f to 40ddef8CompareDecember 2, 2024 03:58
nobu
nobu approved these changes Dec 2, 2024
@hsbt
hsbt merged commit af69c46 into masterDec 2, 2024
@hsbt
hsbt deleted the removed-random-formatter branch December 2, 2024 04:45
@jdelStrother

jdelStrother commented Dec 2, 2024

Copy link
Copy Markdown

FWIW I think this causes problems with Rails on ruby 3.2.

Rails was using SecureRandom.alphanumeric here - https://github.com/rails/rails/blob/4060253ea7bf1a04c7b2368ffc17ddcf991997f9/activesupport/lib/active_support/core_ext/securerandom.rb#L20, but now throws wrong number of arguments (given 2, expected 0..1).

With the removal of SecureRandom.alphanumeric, that falls back to Random.alphanumeric. On ruby 3.2, that only accepts a single argument:

defalphanumeric(n=nil)n=16ifn.nil?choose(ALPHANUMERIC,n)end

@nevans

Copy link
Copy Markdown
Contributor

As far as I can tell, the only functional changes to Random::Formatter between v0.2.0 (which was installed with ruby 3.1.6) and v0.3.2 are:

  • #alphanumeric keyword arg for characters (which affects Rails, as @jdelStrother mentioned)
  • #uuid_v7 (and #uuid_v4 alias)
$ rbenv shell 3.2.6
$ gem install securerandom -v0.3.2Fetching securerandom-0.3.2.gemSuccessfully installed securerandom-0.3.2Parsing documentation for securerandom-0.3.2Installing ri documentation for securerandom-0.3.2Done installing documentation for securerandom after 0 seconds1 gem installed
$ ruby -rsecurerandom -e "puts SecureRandom.uuid_v7"0193889a-f355-76b5-bff8-9e46efa743d0
$ gem install securerandom -v0.4.0Fetching securerandom-0.4.0.gemSuccessfully installed securerandom-0.4.0Parsing documentation for securerandom-0.4.0Installing ri documentation for securerandom-0.4.0Done installing documentation for securerandom after 0 seconds1 gem installed
$ ruby -rsecurerandom -e "puts SecureRandom.uuid_v7"-e:1:in `<main>': undefined method `uuid_v7' for SecureRandom:Module (NoMethodError)puts SecureRandom.uuid_v7 ^^^^^^^^Did you mean? uuid

rafaelfranca added a commit to rails/rails that referenced this pull request Dec 2, 2024
In ruby/securerandom#34, Random::Formatter was
removed from securerandom. This change broke the alphanumeric method
signature in Ruby 3.2 since the `chars` parameter isn't available
there yet.
@hsbt

hsbt commented Dec 3, 2024

Copy link
Copy Markdown
MemberAuthor

@jdelStrother@nevans Thank you for sharing that. I and @nobu couldn't care these cases before removing Random::Formatter. I'll consider them with compatibility.

byroot pushed a commit to rails/rails that referenced this pull request Dec 3, 2024
In ruby/securerandom#34, Random::Formatter was
removed from securerandom. This change broke the alphanumeric method
signature in Ruby 3.2 since the `chars` parameter isn't available
there yet.
byroot pushed a commit to rails/rails that referenced this pull request Dec 3, 2024
In ruby/securerandom#34, Random::Formatter was
removed from securerandom. This change broke the alphanumeric method
signature in Ruby 3.2 since the `chars` parameter isn't available
there yet.
@nevans

nevans commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

@hsbt Would it be possible (this close to the 3.4 release) to create a new random-formatter gem? For full backward compatibility, it should be an explicit dependency of securerandom. I created a proof-of-concept here: https://github.com/nevans/securerandom/tree/random-formatter. It shouldn't be merged into this repo, so I won't create a PR. But here's the diff, for comparison: master...nevans:securerandom:random-formatter

Sign up for freeto 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.

4 participants

@hsbt@jdelStrother@nevans@nobu