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

Frozen string for entire library #75

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

Lewiscowles1986
Copy link
Contributor

@Lewiscowles1986 Lewiscowles1986 commented Aug 12, 2021

Description

Working on the String.new has helped move things forward with a downstream consumer of this library.

When I enable frozen strings for the entire library, the tests fail.

Types of Changes

  • Bug fix (for downstream consumers who do not expect mutation, or attempt to use frozen string literals with more functionality).
  • Breaking change.
  • Maintenance.

This currently removes Ruby 2.0, 2.1, 2.2, 2.3 and 2.4 (early 2013-late 2016)

[Edit] At the request of @ioquatix this now also removes Ruby 2.5 to keep CI simple. For anyone interested in 2.5 see commit and more specifically orphaned commit from fork of this repo

Testing

Failing test summary
Finished in 0.01378 seconds (files took 0.1659 seconds to load)
51 examples, 32 failures

Failed examples:

rspec ./spec/multipart/post/composite_read_io_spec.rb:131 # Multipart::Post::CompositeReadIO test_empty_parts
rspec './spec/multipart/post/composite_read_io_spec.rb[1:1:5]' # Multipart::Post::CompositeReadIO generic io test_read_into_buffer
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:1]' # Multipart::Post::CompositeReadIO composite io test_full_read_from_several_ios
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:2]' # Multipart::Post::CompositeReadIO composite io test_partial_read
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:3]' # Multipart::Post::CompositeReadIO composite io test_partial_read_to_boundary
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:4]' # Multipart::Post::CompositeReadIO composite io test_read_with_size_larger_than_available
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:5]' # Multipart::Post::CompositeReadIO composite io test_read_into_buffer
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:6]' # Multipart::Post::CompositeReadIO composite io test_multiple_reads
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:7]' # Multipart::Post::CompositeReadIO composite io test_read_after_end
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:8]' # Multipart::Post::CompositeReadIO composite io test_read_after_end_with_amount
rspec './spec/multipart/post/composite_read_io_spec.rb[1:2:9]' # Multipart::Post::CompositeReadIO composite io test_second_full_read_after_rewinding
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:1]' # Multipart::Post::CompositeReadIO nested composite io test_full_read_from_several_ios
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:2]' # Multipart::Post::CompositeReadIO nested composite io test_partial_read
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:3]' # Multipart::Post::CompositeReadIO nested composite io test_partial_read_to_boundary
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:4]' # Multipart::Post::CompositeReadIO nested composite io test_read_with_size_larger_than_available
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:5]' # Multipart::Post::CompositeReadIO nested composite io test_read_into_buffer
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:6]' # Multipart::Post::CompositeReadIO nested composite io test_multiple_reads
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:7]' # Multipart::Post::CompositeReadIO nested composite io test_read_after_end
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:8]' # Multipart::Post::CompositeReadIO nested composite io test_read_after_end_with_amount
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:9]' # Multipart::Post::CompositeReadIO nested composite io test_second_full_read_after_rewinding
rspec './spec/multipart/post/composite_read_io_spec.rb[1:3:10]' # Multipart::Post::CompositeReadIO nested composite io test_compatible_with_copy_stream
rspec ./spec/multipart/post/composite_read_io_spec.rb:112 # Multipart::Post::CompositeReadIO unicode composite io test_read_from_multibyte
rspec ./spec/multipart/post/parts_spec.rb:73 # Multipart::Post::Parts::FilePart test_correct_length
rspec ./spec/multipart/post/parts_spec.rb:77 # Multipart::Post::Parts::FilePart test_multibyte_file_length
rspec ./spec/multipart/post/parts_spec.rb:81 # Multipart::Post::Parts::FilePart test_multibyte_filename
rspec ./spec/multipart/post/parts_spec.rb:88 # Multipart::Post::Parts::FilePart test_force_content_type_header
rspec ./spec/net/http/post/multipart_spec.rb:60 # Net::HTTP::Post::Multipart test_form_multipart_body
rspec ./spec/net/http/post/multipart_spec.rb:67 # Net::HTTP::Post::Multipart test_form_multipart_body_with_stringio
rspec ./spec/net/http/post/multipart_spec.rb:73 # Net::HTTP::Post::Multipart test_form_multiparty_body_with_parts_headers
rspec ./spec/net/http/post/multipart_spec.rb:89 # Net::HTTP::Post::Multipart test_form_multipart_body_with_array_value
rspec ./spec/net/http/post/multipart_spec.rb:106 # Net::HTTP::Post::Multipart test_form_multipart_body_with_arrayparam
rspec ./spec/net/http/post/multipart_spec.rb:117 # Net::HTTP::Put::Multipart test_form_multipart_body_put

  • I tested my changes locally.
  • I tested my changes in staging.

By mutating inputs, there is more load on downstream consumers of the library. This might require documentation on safely using the library, or .dup calls within the library

@ioquatix
Copy link
Member

We can drop support for 2.5 if it keeps things simple.

@Lewiscowles1986
Copy link
Contributor Author

Lewiscowles1986 commented Aug 13, 2021

So I am not sure how obvious it is, but the read implementation now requires it's downstream consumer to provide a mutable buf. Do you think this should have an error which is more readable than the failures?

  FrozenError:
   can't modify frozen String: ""
  # ./lib/multipart/post/composite_read_io.rb:34:in `read'

I Was thinking perhaps for this case

The buf (buffer) parameter must not be frozen
# ./lib/multipart/post/composite_read_io.rb:{n}:in `read'

@ioquatix ioquatix merged commit 418fa31 into socketry:master Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants