Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Use non-US dates #1274

Merged
merged 2 commits into from
Nov 7, 2017
Merged

Use non-US dates #1274

merged 2 commits into from
Nov 7, 2017

Conversation

jonathaningram
Copy link
Contributor

@jonathaningram jonathaningram commented Oct 31, 2017

Pulls out the non-US dates change from #1256

Copy link
Contributor

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

thanks for this @jonathaningram! just some small comments.


/**
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to remove valid comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert. One of the reason these kinds of comments are not great is that they become outdated unless updated when the function is updated. When I went to update the comment I realised I was describing what the ~4 line function was doing, so it seemed redundant, so I removed it.

*/
export default function formatDateTime(dateString, timezone = 'Etc/UTC') {
const d = moment(dateString);
const formatDateTime = (str, timezone = 'Etc/UTC') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

str -> dateString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it str because technically the caller can pass any string that is not a date, e.g. formatDateTime('random non date string') and str encapsulates that an arbitrary string was passed. Per https://golang.org/src/time/format.go?s=23626:23672#L762

I can change it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. I can see the case for leaving it as str. Ignore this and just bring back the comment

@jonathaningram
Copy link
Contributor Author

@jcscottiii OK, reinstated the comment. Ready for review again!

@jcscottiii jcscottiii merged commit bd2b8c9 into cloud-gov:master Nov 7, 2017
@jonathaningram jonathaningram deleted the ji-non-us-dates branch November 7, 2017 21:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants