Yep, that sounds good too, Kerri. Artur, go right ahead!
-- Kasper On 10 Jan 2018, 15.37 +0100, Kerri Miller <kerri...@gmail.com>, wrote: > Not to "well actually" myself but another option might be to use > MAX_PASSWORD_LENGTH_ALLOWED as the OP suggested, but have an explicit test > for MAX_PASSWORD_LENGTH_ALLOWED=72.. which might actually be a better > approach, since it'll isolate the exact defect case > (MAX_PASSWORD_LENGTH_ALLOWED != 72) rather than have it be implied by a > well-named-but-oddly-disconnected constant. > > // Kerri > > > On Wed, Jan 10, 2018 at 3:19 PM, <artur.belja...@internet.ee> wrote: > > > I would be happy to do this! > > > > > > > > > On Tuesday, January 9, 2018 at 10:30:41 PM UTC+2, Kasper Timm Hansen > > > wrote: > > > > Yep, we could do that 👍 > > > > > > > > > Den 9. jan. 2018 kl. 12.39 skrev Kerri Miller <kerr...@gmail.com>: > > > > > > > > > > I could see an argument made for making it a constant in the _test_ > > > > > (EXPECTED_MAX_PASSWORD_LENGTH), which at least would document /why/ > > > > > the number is being used. > > > > > > > > > > > On Tue, Jan 9, 2018 at 11:37 AM, Kasper Timm Hansen > > > > > > <kas...@gmail.com> wrote: > > > > > > > I’m -1 on changing that. I think the constant 72 is used directly > > > > > > > such that tests will fail if anybody changes > > > > > > > MAX_PASSWORD_LENGTH_ALLOWED. > > > > > > > > > > > > > > With your change in, a later change to > > > > > > > MAX_PASSWORD_LENGTH_ALLOWED by some other contributor wouldn’t > > > > > > > make any tests fail. > > > > > > > > > > > > > > Since that constant isn’t meant to change without us thinking > > > > > > > more about it, I’d say we keep the code as is. > > > > > > > > > > > > > > Thanks for starting to contribute! > > > > > > > > > > > > > > > Den 9. jan. 2018 kl. 09.56 skrev artur.b...@internet.ee: > > > > > > > > > > > > > > > > Magic number 72 is used many times throughout the tests, which > > > > > > > > is actually present under > > > > > > > > ActiveModel::SecurePassword::MAX_PASSWORD_LENGTH_ALLOWED > > > > > > > > constant. I propose to replace that magic number either by the > > > > > > > > constant or method call. > > > > > > > > > > > > > > > > I have never contributed to such large open source project as > > > > > > > > Rails. Please let me know your thoughts. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/rails/rails/blob/master/activemodel/test/cases/secure_password_test.rb > > > > > > > > https://github.com/rails/rails/blob/ff657e73f0b61a7e224a9f158467ed2ec915bd9b/activemodel/lib/active_model/secure_password.rb > > > > > > > > > > > > > > > > -- > > > > > > > > You received this message because you are subscribed to the > > > > > > > > Google Groups "Ruby on Rails: Core" group. > > > > > > > > To unsubscribe from this group and stop receiving emails from > > > > > > > > it, send an email to rubyonrails-co...@googlegroups.com. > > > > > > > > To post to this group, send email to > > > > > > > > rubyonra...@googlegroups.com. > > > > > > > > Visit this group at > > > > > > > > https://groups.google.com/group/rubyonrails-core. > > > > > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > > > > -- > > > > > > > Kasper > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > You received this message because you are subscribed to the > > > > > > > Google Groups "Ruby on Rails: Core" group. > > > > > > > To unsubscribe from this group and stop receiving emails from it, > > > > > > > send an email to rubyonrails-co...@googlegroups.com. > > > > > > > To post to this group, send email to rubyonra...@googlegroups.com. > > > > > > > Visit this group at > > > > > > > https://groups.google.com/group/rubyonrails-core. > > > > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > > > > > Groups "Ruby on Rails: Core" group. > > > > > To unsubscribe from this group and stop receiving emails from it, > > > > > send an email to rubyonrails-co...@googlegroups.com. > > > > > To post to this group, send email to rubyonra...@googlegroups.com. > > > > > Visit this group at https://groups.google.com/group/rubyonrails-core. > > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > -- > > > > Kasper > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Ruby on Rails: Core" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email to rubyonrails-core+unsubscr...@googlegroups.com. > > > To post to this group, send email to rubyonrails-core@googlegroups.com. > > > Visit this group at https://groups.google.com/group/rubyonrails-core. > > > For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to rubyonrails-core+unsubscr...@googlegroups.com. > To post to this group, send email to rubyonrails-core@googlegroups.com. > Visit this group at https://groups.google.com/group/rubyonrails-core. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at https://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
