https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292017-08-18T17:56:44ZRedmineRedmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=807042017-08-18T17:56:44ZToshi MARUYAMA
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/80704/diff?detail_id=63614">diff</a>)</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=807052017-08-18T18:11:58ZToshi MARUYAMA
<ul><li><strong>Target version</strong> set to <i>4.1.0</i></li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=898362019-02-05T22:39:09ZMarius BĂLTEANU
<ul></ul><p>I've added your patch to my <a href="https://gitlab.com/marius-balteanu/redmine/commits/feature/frozen_string_literals" class="external">GitLab repository</a> in order to run the tests and to see what is missing.</p>
<p>Do you see any value in adding the <code># frozen_string_literal: true</code> to db migration files?</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=898382019-02-05T23:31:15ZPavel Rosický
<ul></ul><p>Hi Marius,<br />the patch is more than a year old, so there'll be some conflicts and missing files.</p>
<p>Do you see any value in adding the # frozen_string_literal: true to db migration files?<br />There was a discussion about making it default for Ruby 3, so it was historically enforced everywhere by formatters like rubocop <a class="external" href="https://github.com/rubocop-hq/rubocop">https://github.com/rubocop-hq/rubocop</a> . But in terms of performance or memory usage there probably no point for adding the comment to migration files.</p>
<p>one more thing, if Ruby 2.2 support is dropped <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Drop Ruby 2.2 support (Closed)" href="https://www.redmine.org/issues/30356">#30356</a><br />instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.</p>
<p>should I update the patch and retest it on the recent redmine version?</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=898422019-02-06T06:23:16ZMarius BĂLTEANU
<ul></ul><p>Pavel Rosický wrote:</p>
<blockquote>
<p>Hi Marius,</p>
<p>one more thing, if Ruby 2.2 support is dropped <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Drop Ruby 2.2 support (Closed)" href="https://www.redmine.org/issues/30356">#30356</a><br />instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.</p>
<p>should I update the patch and retest it on the recent redmine version?</p>
</blockquote>
<p>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.</p>
<p>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.</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=898532019-02-06T20:49:52ZPavel Rosický
<ul></ul><p>ok, rebased and pushed to the github<br /><a class="external" href="https://github.com/redmine/redmine/pull/113">https://github.com/redmine/redmine/pull/113</a></p>
<p>there're many changes but most of them is just adding a magic comment</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906162019-03-06T13:22:13ZPavel Rosický
<ul></ul><p>since ruby 2.2 support was dropped this should be ready for 4.1</p>
<p>let me know if there's anything else I can do</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906222019-03-06T17:09:02ZGo MAEDA
<ul></ul><p>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.</p>
<p>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.</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906232019-03-06T19:29:54ZPavel Rosický
<ul><li><strong>File</strong> <a href="/attachments/22575">frozen.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/22575/frozen.patch">frozen.patch</a> added</li></ul><p><a class="external" href="https://github.com/redmine/redmine/pull/115">https://github.com/redmine/redmine/pull/115</a><br />this is a pull request without frozen_string_literal comment - 69 vs 936 changes, it should be much easier to review</p>
<p>you can apply the comment afterwards using rubocop like this<br /><strong>> rubocop --only Style/FrozenStringLiteralComment --auto-correct -c ruboconfig.yml</strong></p>
<p>ruboconfig.yml<br /><pre>
AllCops:
TargetRubyVersion: 2.3
</pre></p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906282019-03-07T03:04:07ZGo MAEDA
<ul></ul><p>Pavel Rosický wrote:</p>
<blockquote>
<p><a class="external" href="https://github.com/redmine/redmine/pull/115">https://github.com/redmine/redmine/pull/115</a><br />this is a pull request without frozen_string_literal comment - 69 vs 936 changes, it should be much easier to review</p>
</blockquote>
<p>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:</p>
<ul>
<li>Changes don't affect production environments</li>
<li>We can ensure that all code is executed with frozen_string_literals enabled by simply running <code>bin/rails test</code></li>
</ul>
<p>After that, I want to apply the rest of your patch.</p>
<p>Comments welcome.</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906462019-03-07T20:37:27ZPavel Rosický
<ul></ul><p>Go MAEDA wrote:</p>
<blockquote>
<p>Thank you for updating the patch. I would like to enable frozen_string_literal in files under the ./test directory first</p>
</blockquote>
<p>I can do that, but there're some problematic methods<br />User#find_by_login<br /><a class="external" href="https://github.com/redmine/redmine/blob/33522bf2e88026d0ca4f3d304d016dddf5966044/app/models/user.rb#L489">https://github.com/redmine/redmine/blob/33522bf2e88026d0ca4f3d304d016dddf5966044/app/models/user.rb#L489</a><br />Redmine::FieldFormat#parse_keyword<br /><a class="external" href="https://github.com/redmine/redmine/blob/4770fa5b6a12f78a5c055efc8a7bf47555496426/lib/redmine/field_format.rb#L200">https://github.com/redmine/redmine/blob/4770fa5b6a12f78a5c055efc8a7bf47555496426/lib/redmine/field_format.rb#L200</a><br />and the whole codeset_utils</p>
<p>they're are used heavily in tests</p>
<p>consider this example<br /><pre>
login = 'test'.force_encoding('ascii-8bit')
puts "before: #{login.encoding}" # => ascii-8bit
user = User.find_by_login(login)
puts "after: #{login.encoding}" # => utf-8
</pre></p>
<p>or in my case<br /><pre>
login = 'test'.freeze
User.find_by_login(login)
=> FrozenError: can't modify frozen String
</pre></p>
<p>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?</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906542019-03-07T23:58:49ZGo MAEDA
<ul></ul><p>Pavel Rosický wrote:</p>
<blockquote>
<p>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?</p>
</blockquote>
<p>You don't have to work on splitting the patch. I will do that :)</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906552019-03-08T00:12:40ZPavel Rosický
<ul></ul><p>great, thanks :)</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=906852019-03-10T13:45:14ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-3 status-5 priority-4 priority-default closed" href="/issues/31004">Patch #31004</a>: Decode hexadecimal-encoded literals in order to be frozen string literals friendly</i> added</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908072019-03-19T02:37:32ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/31052">Defect #31052</a>: Download unified diff throws "can't modify frozen String" error.</i> added</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908242019-03-19T23:24:47ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-3 status-5 priority-4 priority-default closed" href="/issues/31059">Patch #31059</a>: Use #b shortcut instead of #force_encoding</i> added</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908522019-03-21T06:23:14ZGo MAEDA
<ul></ul><p>Thanks to Pavel Rosický, I have enabled frozen_string_literal for almost all files used when running the application or tests.</p>
<p>But I have not enabled frozen_string_literal for some files yet for the following reasons:</p>
<ul>
<li>lib/redmine/imap.rb and lib/redmine/pop3.rb are not covered by existing tests</li>
<li>I don't have environments to test Redmine.pm or OpenId</li>
</ul>
<p>Here is a list of files for which frozen_string_literal is still "false":</p>
<ul>
<li>extra/svn/reposman.rb</li>
<li>extra/mail_handler/rdm-mailhandler.rb</li>
<li>lib/redmine/imap.rb</li>
<li>lib/redmine/pop3.rb</li>
<li>lib/plugins/open_id_authentication/init.rb</li>
<li>lib/plugins/open_id_authentication/lib/open_id_authentication/mem_cache_store.rb</li>
<li>lib/plugins/open_id_authentication/lib/open_id_authentication/request.rb</li>
<li>lib/plugins/open_id_authentication/lib/open_id_authentication/db_store.rb</li>
<li>test/extra/redmine_pm/repository_git_test_pm.rb</li>
<li>test/extra/redmine_pm/test_case.rb</li>
<li>test/extra/redmine_pm/repository_subversion_test_pm.rb</li>
</ul>
<p>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.</p>
<p>I hope someone tests the above files and enables frozen_string_literal for those in the future.</p> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908532019-03-21T06:23:28ZGo MAEDA
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908542019-03-21T06:24:15ZGo MAEDA
<ul><li><strong>Subject</strong> changed from <i>Frozen string literals</i> to <i>Enable frozen string literals</i></li><li><strong>Assignee</strong> set to <i>Go MAEDA</i></li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=908942019-03-24T01:24:07ZGo MAEDA
<ul><li><strong>Status</strong> changed from <i>Resolved</i> to <i>Closed</i></li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=937342019-09-16T09:12:20ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/32071">Defect #32071</a>: FrozenError in assert_save in test/test_helper.rb</i> added</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=947322019-11-02T13:53:58ZMarius BĂLTEANU
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/31508">Defect #31508</a>: Add missing frozen strings and copyrights</i> added</li></ul> Redmine - Feature #26561: Enable frozen string literalshttps://www.redmine.org/issues/26561?journal_id=1060872022-03-22T06:36:36ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/33021">Defect #33021</a>: [v.4.0.5-stable] Internal Server Error 500 when accessing 'repository' tab</i> added</li></ul>