Redmine

I've added your patch to my GitLab repository in order to run the tests and to see what is missing.

Do you see any value in adding the # frozen_string_literal: true to db migration files?

Hi Marius,
the patch is more than a year old, so there'll be some conflicts and missing files.

Do you see any value in adding the # frozen_string_literal: true to db migration files?
There was a discussion about making it default for Ruby 3, so it was historically enforced everywhere by formatters like rubocop https://github.com/rubocop-hq/rubocop . But in terms of performance or memory usage there probably no point for adding the comment to migration files.

one more thing, if Ruby 2.2 support is dropped #30356
instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.

should I update the patch and retest it on the recent redmine version?

Pavel Rosický wrote:

Hi Marius,

one more thing, if Ruby 2.2 support is dropped #30356
instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.

should I update the patch and retest it on the recent redmine version?

Yes, if you can do it, it will be very useful and I think we should have in mind that the support for Ruby 2.2 will be dropped in 4.1.

I'm in favour of this change, but this doesn't mean necessary that the patch will be committed. Most probably, it will be Jean-Philippe decision, but considering that almost all gems use frozen_string_literal, I don't see a real reason for not including it.

ok, rebased and pushed to the github
https://github.com/redmine/redmine/pull/113

there're many changes but most of them is just adding a magic comment

since ruby 2.2 support was dropped this should be ready for 4.1

let me know if there's anything else I can do

Since the patch is too big to review for me, I would like to take a step-by-step approach. It means that I will enable frozen_string_literal in each file one after another.

Today, I confirmed that we can enable frozen_string_literal for test/unit/activity_test.rb by just adding a magic comment and for test/unit/attachment_test.rb by applying Pavel Rosický's patch.

Pavel Rosický wrote:

https://github.com/redmine/redmine/pull/115
this is a pull request without frozen_string_literal comment - 69 vs 936 changes, it should be much easier to review

Thank you for updating the patch. I would like to enable frozen_string_literal in files under the ./test directory first for the following reasons:

• Changes don't affect production environments
• We can ensure that all code is executed with frozen_string_literals enabled by simply running bin/rails test

After that, I want to apply the rest of your patch.

Comments welcome.

Go MAEDA wrote:

Thank you for updating the patch. I would like to enable frozen_string_literal in files under the ./test directory first

I can do that, but there're some problematic methods
User#find_by_login
https://github.com/redmine/redmine/blob/33522bf2e88026d0ca4f3d304d016dddf5966044/app/models/user.rb#L489
Redmine::FieldFormat#parse_keyword
https://github.com/redmine/redmine/blob/4770fa5b6a12f78a5c055efc8a7bf47555496426/lib/redmine/field_format.rb#L200
and the whole codeset_utils

they're are used heavily in tests

consider this example

login = 'test'.force_encoding('ascii-8bit')
puts "before: #{login.encoding}" # => ascii-8bit
user = User.find_by_login(login)
puts "after: #{login.encoding}" # => utf-8

or in my case

login = 'test'.freeze
User.find_by_login(login)
=> FrozenError: can't modify frozen String

What is the prefered way how to deal with these issues? I would rather fix those methods to avoid side-effects, what do you think?

Pavel Rosický wrote:

What is the prefered way how to deal with these issues? I would rather fix those methods to avoid side-effects, what do you think?

You don't have to work on splitting the patch. I will do that :)

great, thanks :)

Thanks to Pavel Rosický, I have enabled frozen_string_literal for almost all files used when running the application or tests.

But I have not enabled frozen_string_literal for some files yet for the following reasons:

• lib/redmine/imap.rb and lib/redmine/pop3.rb are not covered by existing tests
• I don't have environments to test Redmine.pm or OpenId

Here is a list of files for which frozen_string_literal is still "false":

• extra/svn/reposman.rb
• extra/mail_handler/rdm-mailhandler.rb
• lib/redmine/imap.rb
• lib/redmine/pop3.rb
• lib/plugins/open_id_authentication/init.rb
• lib/plugins/open_id_authentication/lib/open_id_authentication/mem_cache_store.rb
• lib/plugins/open_id_authentication/lib/open_id_authentication/request.rb
• lib/plugins/open_id_authentication/lib/open_id_authentication/db_store.rb
• test/extra/redmine_pm/repository_git_test_pm.rb
• test/extra/redmine_pm/test_case.rb
• test/extra/redmine_pm/repository_subversion_test_pm.rb

However, I think I can close this issue because frozen_string_literal is enabled for 98% of files and features implemented by the above files are not so popular.

I hope someone tests the above files and enables frozen_string_literal for those in the future.