Magic number in tests

[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

[Kasper Timm Hansen]

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!

--
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

[Kerri Miller]

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-core+unsubscribe@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.

--

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+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.

[Kasper Timm Hansen]

Yep, we could do that 👍

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

[artur.b...@internet.ee]

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

[artur.b...@internet.ee]

Thank you for feedback!

[Kerri Miller]

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

To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.

[Kasper Hansen]

Yep, that sounds good too, Kerri.

Artur, go right ahead!

--
Kasper

[Artur Beljajev]

Should I create a ticket on Github first?

[Kasper Hansen]

Artur, send a pull request :)

--
Kasper