- 
                Notifications
    
You must be signed in to change notification settings  - Fork 123
 
Implement parsing and formatting for Instant as separate functions #411
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
Conversation
71fc711    to
    fd78536      
    Compare
  
    | total += (y + 3) / 4 - (y + 99) / 100 + (y + 399) / 400 | ||
| } else { | ||
| total -= y / -4 - y / -100 + y / -400 | 
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.
Looks like a case for ceilDiv in stdlib :)
fd78536    to
    019c71b      
    Compare
  
    | private fun parseInstant(isoString: String): Instant { | ||
| // return Instant.parse(isoString) | ||
| return parseIso(isoString) | ||
| } | ||
| 
               | 
          ||
| private fun displayInstant(instant: Instant): String { | ||
| // return instant.toString() | ||
| return formatIso(instant) | ||
| } | 
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.
@qurbonzoda, here's the logic behind this PR: the newly-added formatIso can replace toString, and the newly-added parseIso can replace Instant.parse in the case when no specific format is passed (which is the only case supported by `kotlin.time.Instant).
023b7ac    to
    ece4e11      
    Compare
  
            
          
                core/commonKotlin/src/Instant.kt
              
                Outdated
          
        
      | expect("'T' or 't'", i + 6) { it == 'T' || it == 't' } | ||
| expect("':'", i + 9) { it == ':' } | ||
| expect("':'", i + 12) { it == ':' } | ||
| for (j in listOf(1, 2, 4, 5, 7, 8, 10, 11, 13, 14)) { | 
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.
Better to extract such index lists to variables outside the function and (optionally) make them arrays.
f77c2a0    to
    3de6ba9      
    Compare
  
    | override fun toString(): String = "UnboundedLocalDateTime($year-$month-$day $hour:$minute:$second.$nanosecond)" | ||
| 
               | 
          ||
| companion object { | ||
| fun fromInstant(instant: Instant, offsetSeconds: Int): UnboundedLocalDateTime { | 
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.
There is only one call to this function, and it passes 0 for offsetSeconds.
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.
Yes. Do you see any problems with that?
| fun parseFailure(error: String): Nothing { | ||
| throw IllegalArgumentException("$error when parsing an Instant from $isoString") | ||
| } | ||
| inline fun expect(what: String, where: Int, predicate: (Char) -> Boolean) { | 
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.
Local inline functions are not yet supported.
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.
When I build the project or open it in the IDE, I receive no such error/warning.
c01263f    to
    5882b5c      
    Compare
  
    4c4e638    to
    70469f0      
    Compare
  
    | 
           This code is in some form already in Kotlin's   | 
    
Implement parsing and formatting of ISO strings for instant values separately from the rest of the library.
A stage of fixing #382
This is a draft PR because we probably don't need to actually merge it into the datetime library. However, the code is already written and can be reviewed.