Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update for frozen string literals #1101

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/chat.rb
@@ -1,5 +1,6 @@
#!/usr/bin/env ruby -I ../lib -I lib
# coding: utf-8
# frozen_string_literal: true
require 'sinatra'
set :server, 'thin'
connections = []
Expand Down
1 change: 1 addition & 0 deletions examples/simple.rb
@@ -1,3 +1,4 @@
#!/usr/bin/env ruby -I ../lib -I lib
# frozen_string_literal: true
require 'sinatra'
get('/') { 'this is a simple app' }
1 change: 1 addition & 0 deletions lib/sinatra.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed for the one string allocated here "sinatra/main" is an acceptable allocation that I doubt will be repeated enough to affect performance

require 'sinatra/main'

enable :inline_templates
10 changes: 8 additions & 2 deletions lib/sinatra/base.rb
Expand Up @@ -348,7 +348,7 @@ def attachment(filename = nil, disposition = :attachment)
response['Content-Disposition'] = disposition.to_s
if filename
params = '; filename="%s"' % File.basename(filename)
response['Content-Disposition'] << params
response['Content-Disposition'] += params
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, I'm not sure this change is necessary as we haven't seen any failures in this case

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzak I just ran across this in one of our projects. See #1137. We tried to freeze the constant we were using to set dispositions and got lots of errors..

Also this fixed a possible unintended side effect bug. If you hold a reference to the string you pass as the disposition, you'll find that string changed after the call.

x = 'inline'
send_file(some_path, disposition: x)
x == 'inline' #=> false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aselder Thanks for your investigation!

I will consider your patch, and this one but unfortunately I don't want to merge all of these comments in the test files.

ext = File.extname(filename)
content_type(ext) unless response['Content-Type'] or ext.empty?
end
Expand Down Expand Up @@ -842,6 +842,7 @@ def compile_template(engine, data, options, views)
body, path, line = settings.templates[data]
if body
body = body.call if body.respond_to?(:call)
body = body.dup if body.frozen?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a case where body is actually immutable from here? Maybe an empty request?

template.new(path, line.to_i, options) { body }
else
found = false
Expand All @@ -857,7 +858,12 @@ def compile_template(engine, data, options, views)
template.new(path, 1, options)
end
when Proc, String
body = data.is_a?(String) ? Proc.new { data } : data
body = if data.is_a?(String)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

data = data.dup if data.frozen?
Proc.new { data }
else
data
end
path, line = settings.caller_locations.first
template.new(path, line.to_i, options, &body)
else
Expand Down
1 change: 1 addition & 0 deletions lib/sinatra/main.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require 'sinatra/base'

module Sinatra
Expand Down
1 change: 1 addition & 0 deletions lib/sinatra/version.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
module Sinatra
VERSION = '2.0.0-alpha'
end
1 change: 1 addition & 0 deletions test/asciidoctor_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/base_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class BaseTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/builder_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/coffee_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/compile_test.rb
@@ -1,4 +1,5 @@
# I like coding: UTF-8
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class CompileTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/contest.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
# Copyright (c) 2009 Damian Janowski and Michel Martens for Citrusbyte
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
Expand Down
1 change: 1 addition & 0 deletions test/creole_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/delegator_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class DelegatorTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/encoding_test.rb
@@ -1,4 +1,5 @@
# encoding: UTF-8
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require 'erb'

Expand Down
1 change: 1 addition & 0 deletions test/erb_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class ERBTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/extensions_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class ExtensionsTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/filter_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class BeforeFilterTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/haml_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/helper.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
ENV['RACK_ENV'] = 'test'
Encoding.default_external = "UTF-8" if defined? Encoding

Expand Down
1 change: 1 addition & 0 deletions test/helpers_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require 'date'
require 'json'
Expand Down
1 change: 1 addition & 0 deletions test/integration/app.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
$stderr.puts "loading"
require 'sinatra'

Expand Down
1 change: 1 addition & 0 deletions test/integration_helper.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require 'sinatra/base'
require 'rbconfig'
require 'open-uri'
Expand Down
1 change: 1 addition & 0 deletions test/integration_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require File.expand_path('../integration_helper', __FILE__)

Expand Down
1 change: 1 addition & 0 deletions test/less_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/liquid_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/mapped_error_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class FooError < RuntimeError
Expand Down
1 change: 1 addition & 0 deletions test/markaby_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/markdown_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

MarkdownTest = proc do
Expand Down
1 change: 1 addition & 0 deletions test/mediawiki_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/middleware_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class MiddlewareTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/nokogiri_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/rabl_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/rack_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require 'rack'

Expand Down
1 change: 1 addition & 0 deletions test/radius_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/rdoc_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/readme_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
# Tests to check if all the README examples work.
require File.expand_path('../helper', __FILE__)

Expand Down
1 change: 1 addition & 0 deletions test/request_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require 'stringio'

Expand Down
1 change: 1 addition & 0 deletions test/response_test.rb
@@ -1,4 +1,5 @@
# encoding: utf-8
# frozen_string_literal: true

require File.expand_path('../helper', __FILE__)

Expand Down
1 change: 1 addition & 0 deletions test/result_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class ResultTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/route_added_hook_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

module RouteAddedTest
Expand Down
1 change: 1 addition & 0 deletions test/routing_test.rb
@@ -1,4 +1,5 @@
# I like coding: UTF-8
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

# Helper method for easy route pattern matching testing
Expand Down
1 change: 1 addition & 0 deletions test/sass_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/scss_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/server_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
require 'stringio'

Expand Down
1 change: 1 addition & 0 deletions test/settings_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class SettingsTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/sinatra_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class SinatraTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/slim_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/static_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class StaticTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/streaming_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

class StreamingTest < Minitest::Test
Expand Down
1 change: 1 addition & 0 deletions test/stylus_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/templates_test.rb
@@ -1,4 +1,5 @@
# encoding: UTF-8
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)
File.delete(File.dirname(__FILE__) + '/views/layout.test') rescue nil

Expand Down
1 change: 1 addition & 0 deletions test/textile_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/views/ascii.erb
@@ -1,2 +1,3 @@
<%# frozen_string_literal: true %>
This file has no unicode in it!
<%= value %>
3 changes: 2 additions & 1 deletion test/views/calc.html.erb
@@ -1 +1,2 @@
<%= 1 + 1 %>
<%# frozen_string_literal: true %>
<%= 1 + 1 %>
1 change: 1 addition & 0 deletions test/views/error.erb
@@ -1,3 +1,4 @@
<%# frozen_string_literal: true %>
Hello <%= 'World' %>
<% raise 'Goodbye' unless defined?(french) && french %>
<% raise 'Au revoir' if defined?(french) && french %>
1 change: 1 addition & 0 deletions test/views/error.haml
@@ -1,3 +1,4 @@
-# frozen_string_literal: true
%h1 Hello From Haml
= raise 'goodbye' unless defined?(french) && french
= raise 'au revoir' if defined?(french) && french
1 change: 1 addition & 0 deletions test/views/hello.erb
@@ -1 +1,2 @@
<%# frozen_string_literal: true %>
Hello <%= 'World' %>
1 change: 1 addition & 0 deletions test/views/hello.haml
@@ -1 +1,2 @@
-# frozen_string_literal: true
%h1 Hello From Haml
1 change: 1 addition & 0 deletions test/views/layout2.erb
@@ -1,2 +1,3 @@
<%# frozen_string_literal: true %>
ERB Layout!
<%= yield %>
1 change: 1 addition & 0 deletions test/views/layout2.haml
@@ -1,2 +1,3 @@
-# frozen_string_literal: true
%h1 HAML Layout!
%p= yield
1 change: 1 addition & 0 deletions test/views/utf8.erb
@@ -1,2 +1,3 @@
<%# frozen_string_literal: true %>
<h1><%= value %></h1>
Ingen vill veta var du köpt din tröja.
1 change: 1 addition & 0 deletions test/wlang_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down
1 change: 1 addition & 0 deletions test/yajl_test.rb
@@ -1,3 +1,4 @@
# frozen_string_literal: true
require File.expand_path('../helper', __FILE__)

begin
Expand Down