wordpress-mobile / WordPress-iOS

WordPress for iOS - Official repository
http://ios.wordpress.org/
GNU General Public License v2.0
3.7k stars 1.12k forks source link

Thinking out loud: Micro-framework for `DateFormatter`s? #21977

Open mokagio opened 1 year ago

mokagio commented 1 year ago

I randomly bumped in a DateFormatter configuration that looked familiar in a code review, https://github.com/wordpress-mobile/WordPress-iOS/pull/21964/files#diff-e26a4b685d632fa3deaecee5441e8e9aff7027dc6bb7f7c9b899c85e5145110eR72-R77

I quick find in Xcode revealed three exact matches (I was on release/23.5.2).

image

I wouldn't be surprised if there were more with a different syntax, and I have a feeling that other formatter configurations are similarly duplicated.

Would it make sense to 1) centralize these configuration... 2) in a framework or Swift package?

I'm thinking out loud here and I'm not sure whether the setup effort and the import <framework name> overhead would be worth avoiding the repetition. I guess the idea popped to mind because we've been talking about modularization as a way to reduce complexity, help testability, and increase quality recently.

I believe modularization is a good investment to improve those metrics. But not all modules are created equal and one needs to be mindful of the boundaries they define. Also, micro-frameworks sound attractive, but they can be a slippery slope that leads to granularity for its own sake and mental burden to trace the dependency tree.

We could settle for simply removing duplication with a WordPress & Jetpack level DateFormatter utility class.

Also worth noting that WordPressShared has already done a lot of this work, https://github.com/wordpress-mobile/WordPress-iOS-Shared/blob/4536c4058e2ffbe7281635f1586b2930c7121225/Sources/WordPressShared/Utility/NSDate%2BHelpers.swift#L1-L69, but for some reason it's all private. Maybe we could bypass the setup cost by simply making that code public? One risk in this, though, is that the framework is shared. Delegating presentation logic details to a shared framework could backfire is another consumer modifies them.

peril-wordpress-mobile[bot] commented 1 year ago
Fails
:no_entry_sign: Please add a feature label to this issue. e.g. 'Stats'

Generated by :no_entry_sign: dangerJS

mokagio commented 1 year ago

Trying to look deeper at the problem.

We have duplication in how the same DateFormatter is configured across the app.

Why? Various reasons, including lack of discoverability of previous configurations. But also, we have a bunch of different DateFormatters.

Do we even need that many different configurations?

kean commented 1 year ago

centralize these configuration

DateFormatter is a high-level API, and there are too many possible combinations, making it hard to come up with good names for the factory methods.

But it lacks some convenience APIs. So instead of these:

       static let mediumDate: DateFormatter = {
            let formatter = DateFormatter()
            formatter.dateStyle = .medium
            formatter.timeStyle = .none
            return formatter
        }()

        // No clear from the name that it does relative formatting. Adding _every_ parameter to the name 
       // is also counterproductive. 
        static let mediumDateTime: DateFormatter = {
            let formatter = DateFormatter()
            formatter.doesRelativeDateFormatting = true
            formatter.dateStyle = .medium
            formatter.timeStyle = .short
            return formatter
        }()

It's be great to be able to write this:

DateFormatter(dateStyle: .medium, timeStyle: .short)

There are likely some formatters that are not used and some that could be replaced with more modern variants, e.g. static let iso8601: DateFormatter with ISO8601DateFormatter.

I'm thinking out loud here and I'm not sure whether the setup effort and the import overhead

For types, you can avoid imports by using typealises. For example, instead of (imaginary) import WPAnalyticsKit, you could add a typealis in one of the project files and have it work everywhere.

typealias WPAnalytics = WPAnalyticsKit.WPAnalytics

P.S. I'm not sure how important it is to cache the formatters. It used to be. I usually define static properties myself if it's for table cells, for example.

mokagio commented 1 year ago

For types, you can avoid imports by using typealises

TIL. That's neat.