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

ISO8601DateFormatter #77

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dpfannenstiel
Copy link

As a web site developer I want to support time zone within my posts without having to write a custom date formatter. The default date formatter has support for the format "yyyy-MM-dd HH:mm", which ignores time zone.

In Publish 0.5.0 date formatting is handled via the DateFormatter class, which conforms to the Formatter protocol. Along with the many fine date formats there exists ISO 8601, which the Foundation framework implements in the ISO8601DateFormatter class, which also conforms to the Formatter protocol. Since PublishingContext requires a DateFormatter I cannot use ISO8601DateFormatter.

This pull request makes two changes.

First, it introduces AnyDateFormatter, a minimal protocol to define a common interface for date formatters to conform. Most of the API is already implemented by DateFormatter and ISO8601DateFormatter, so implementing the protocol as an extension is trivial.

Second, PublishingContext changes dateFormatter to a AnyDateFormatter and then make any subsequent changes to support that type change. As part of this change, the default dateFormatter value is set to a ISO8601DateFormatter. This will result in a breaking change for existing users of Publish, but it is believed that using a well known date format represents a common initial case.

@john-mueller
Copy link
Contributor

I think the addition of AnyDateFormatter and ISO 8601 compatibility make sense. I don't think making it the default is the best option, however, as I think most people would rather use the current default "2020-03-12 15:00", as opposed to "2020-03-12T15:00:00-06:00".

You could use ISO 8601 with a custom step:

.step(named: "Use ISO8601DateFormatter") { context in
    if #available(OSX 10.12, *) {
        context.dateFormatter = ISO8601DateFormatter()
    }
}

It's probably worth adding that to Documentation/HowTo/using-a-custom-date-formatter.md.

@dpfannenstiel
Copy link
Author

I've updated the PR with better documentation on AnyDateFormatter.

My thoughts regarding the default date formatter are based around standards. ISO 8601 has really wide adoption, whereas the current default is pretty custom and doesn't handle timezone at all. I don't know what the roadmap to 1.0 looks like, but thinking about onboarding a new user you want to give them the minimum amount of friction to deployment. So I guess it becomes a question of how important is time zone localization to a mature product for deployment.

Copy link
Owner

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great change, thanks @dpfannenstiel! I added a few comments inline that will need to be addressed before this can be merged in, but otherwise I'd be happy to make this a part of Publish's official API 👍

func string(from date: Date) -> String
}

/// Apply `AnyDateFormatter` to `DateFormatter`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unnecessary, since the code is completely self-explanatory.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digging into the PR notes I found a (better) version of this protocol exists as part of your Codextended library. The major difference is that this version conforms to Formatter. I'm pretty comfortable dropping that requirement.

I still think that AnyDateFormatter has a valuable place in Publish to support ISO 8601. Including this protocol is redundant. Not including it requires an @import Codextended anyplace we use it, regardless of other comments. I'd like your thoughts.

func date(from string: String) -> Date?
/// Returns a string representation of a given date formatted using the receiver’s current settings.
/// - parameter date: The date to format.
func string(from date: Date) -> String
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is not used within Publish, so including it feels a bit like premature generalization, and makes the AnyDateFormatter protocol more complex than it has to be.

/// The time zone for the receiver.
var timeZone: TimeZone! { get set }
/// The date format string used by the receiver.
var dateFormat: String! { get }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is not used within Publish, so including it feels a bit like premature generalization, and makes the AnyDateFormatter protocol more complex than it has to be.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually used in MarkdownMetadataDecoder here.

/// The date format string used by the receiver.
var dateFormat: String! { get }

/// Returns a date representation of a given string interpreted using the receiver’s current settings.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation seems to be copied from Apple, which is not allowed. I'd recommend coming up with an original, simpler description of the method.

//
// Created by Dustin Pfannenstiel on 3/10/20.
// Copyright (c) Dustin Pfannenstiel, 2020
// MIT license, see LICENSE file for details
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the same start-of-file header as the rest of the project uses (but with your copyright, of course).

/// Apply `AnyDateFormatter` to `DateFormatter`
extension DateFormatter: AnyDateFormatter {}

/// Apply `AnyDateFormatter` to `ISO8601DateFormatter`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, this comment is not required and should be removed.

@available(OSX 10.12, *)
extension ISO8601DateFormatter: AnyDateFormatter {
/// Since `ISO8601DateFormatter` does not have a variable format, return a descriptive string for reporting.
public var dateFormat: String! {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better solution would be to remove this from the protocol requirements, as recommended above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had two reasons for including this. Initially because in at least one place Publish reads a dateFormat out of a AnyDateFormatter.
The second reason is that from an API design perspective what AnyDateFormatter is doing is defining the minimum standard interface for a date formatter. A data formatter without read access to a date format didn't make sense to me. If anything ISO8601DateFormatter represented a special case that needed to be handled. This way instead of having to special case date formatter every time we need the dateFormat we now know the property is available.

@@ -45,6 +45,7 @@ public struct PublishingContext<Site: Website> {
self.folders = folders
self.stepName = firstStepName


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this whitespace.

@JohnSundell JohnSundell added the awaiting update Awaiting an update from the PR author label Dec 30, 2020
@addisonwebb
Copy link

This is something I would love to have merged. Is there any way I can help out with this @dpfannenstiel?

@dpfannenstiel
Copy link
Author

@addisonwebb I’m trying to get @JohnSundell thoughts on how to include the AnyDateFormatter. Digging around I had found a version of it in a support library. But modifications are required. I’m not a huge fan of duplicating code needlessly and Codextended seems like the place to do it.

@addisonwebb
Copy link

Ok @dpfannenstiel @JohnSundell I have submitted 2 PRs to address this issue using the existing AnyDateFormatter in Codextended.

These should be the minimal changes necessary to allow Publish users to use both DateFormatter and ISO8601DateFormatter as the custom date formatter on the publishing context. No breaking changes are introduced by these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting update Awaiting an update from the PR author
Projects
None yet
4 participants