- 
                Notifications
    You must be signed in to change notification settings 
- Fork 205
          SF-0033: Implement String.Encoding.ianaName and String.Encoding(ianaName:).
          #1286
        
          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
base: main
Are you sure you want to change the base?
Conversation
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
dd0b16c    to
    1ae2a4c      
    Compare
  
    
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
1ae2a4c    to
    4d87ed8      
    Compare
  
    String.Encoding.ianaName and String.Encoding(ianaName:).String.Encoding.ianaName and String.Encoding(ianaName:).
      
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
217c46d    to
    9cd5c9a      
    Compare
  
    
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
9cd5c9a    to
    3e9aef4      
    Compare
  
    3e9aef4    to
    ac42127      
    Compare
  
    | @swift-ci Please test | 
| I've made this PR ready for review. cc: Foundation Workgroup | 
        
          
                Sources/FoundationEssentials/String/String+Encoding+Names.swift
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                Sources/FoundationEssentials/String/String+Encoding+Names.swift
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @itingliu | 
0045a48    to
    e674fa6      
    Compare
  
    b7702b3    to
    8f84db7      
    Compare
  
    | @swift-ci Please test | 
| @swift-ci test | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me after the recent simplifications. Will also want @itingliu approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like this simplified version. Thank you! just had a few minor comments
        
          
                Sources/FoundationEssentials/String/String+Encoding+Names.swift
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | /// to determine which encoding is suitable. | ||
| @available(FoundationPreview 6.3, *) | ||
| public init?(ianaName charsetName: String) { | ||
| let possibilities: [String.Encoding] = [ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work if you write
let possibilities: [IANACharset] = {...
and below just do
         func __determineEncoding() -> String.Encoding? {
            for ianaCharset in possibilities {
                if ianaCharset.matches(charsetName) {
                    return encoding
                }
            }
            return nil
        }
Am I missing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't we lose encoding for return encoding in that implementation?
| return nil | ||
| } | ||
|  | ||
| guard let encoding = __determineEncoding() else { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to simply this even further, it looks like this logic inside func __determineEncoding() doesn't even needed to be extracted in a function since it's only used once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why nested function is used is just because of readability since self = nil; return is invalid in Swift.
Sorry, we can write self = encoding; return...
I'm fixing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed String.Encoding(ianaName:).
I hope that this is in line with your intention.
Implementation of the proposal: #1243 (SF-0033)