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

IO::EncodingOptions and invalid character actions #11020

Open
HertzDevil opened this issue Jul 26, 2021 · 1 comment
Open

IO::EncodingOptions and invalid character actions #11020

HertzDevil opened this issue Jul 26, 2021 · 1 comment

Comments

@HertzDevil
Copy link
Contributor

IO::EncodingOptions is a weird type. It is marked as public, but is essentially a record with 2 fields that are used exclusively by the private types IO::Encoder and IO::Decoder, wrappers around the iconv interface. There are no public interfaces that produce or consume IO::EncodingOptions values at all.

That type's #invalid field is even weirder (this time the same goes for public methods like String#encode and File.tempfile). It is a Symbol? and the only acceptable values are nil and :skip. This sounds like an opportunity to introduce an enum:

enum String::InvalidAction # alternatively `String::InvalidByteSequenceAction`
  Raise # an exception is raised on invalid byte sequences
  Skip  # invalid byte sequences are ignored

  # :nodoc:
  def self.from_invalid(invalid)
    if invalid.nil?
      Raise
    elsif invalid == :skip
      Skip
    else
      raise ArgumentError.new "Valid values for `invalid` option are `nil` and `:skip`, not #{invalid.inspect}"
    end
  end
end

The overloads that take a Symbol? should be deprecated in favor of this enum:

class String
  def encode(encoding : String, invalid : String::InvalidAction = :raise) : Bytes; ...; end

  @[Deprecated]
  def encode(encoding : String, invalid : Nil) : Bytes
    encode(encoding, String::InvalidAction.from_invalid(invalid))
  end

  # How to we deprecate `Symbol?` and restriction-less overloads properly?
  # (they will be matched for `invalid: :skip`, but we still want to support autocasting here)
end

IO::EncodingOptions can be deprecated too, and IO::Encoder / IO::Decoder should use an equivalent internal type, unless there is a reason to support this struct in the public interface of encoding-related methods.

@asterite
Copy link
Member

EncodingOptions is meant to be nodoc or private.

I think we used symbol before we had enums or autocasting, not sure. The idea was to have other options than skip,eventually.

I don't think this can be changed if someone had a runtime nil or Symbol. That was passed to the method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants