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

Date Manipulating/Formatting Functions #538

Closed
skatcat31 opened this issue Jan 12, 2018 · 34 comments
Closed

Date Manipulating/Formatting Functions #538

skatcat31 opened this issue Jan 12, 2018 · 34 comments

Comments

@skatcat31
Copy link
Contributor

Should we allow Date or time formatting related functions in the main repo?

While ubiquitous I feel like they would more belong in the archive section due to the reliance on a very confusing library and regional/national standards.

@skatcat31 skatcat31 changed the title Date Manipulating Functions Date Manipulating/Formatting Functions Jan 12, 2018
@Chalarangelo
Copy link
Owner

I suspect this is in regards to #536 as well as other things such as toEnglishDate etc. My opinion in general is that our Date category is a bit empty and could use a few snippets, but just a few as date manipulation is a terrifying beast.

As far as #536 is concerned, I believe that at least two of the snippets could be combined and that we could add them to the main repository as they have valid use-cases for people who need to do some Date string manipulation without having to include a library.

My opinion about date manipulation/format functions is that we should consider these factors:

  1. Is the snippet useful and could it cover a wide enough use-case?
  2. Is the snippet easy to understand without having to read about Date libraries/formatting/[any other date-specific stuff you need to google]?
  3. Does the snippet have confusing edge-cases or could lead to unexpected results?

Now, (1) is not always obvious, so this is why we review PRs and decided what we want to keep. (2) and (3) should be quite obvious just from reading the code - if something has to use ISO8601 or whatever, we should avoid it, if we return a result that is not expected (like we could get from toEnglishDate time travelling) we should avoid it. Fair play would be using just simple Date methods and properties, without having to go into technical stuff.

Based on this opinion, here's my proposal for #536 (which I will also comment there):

  • toMeridiem and to12Hour should be merged to provide a back and forth converter (something like convertTime). This would ensure the maximum functionality possible and could be really useful to a lot of people that want to avoid using a library. It's simple math, it's easy to understand and gets the job done without having people look to external resources.
  • toColonTime should be called something like prettyTime and its possible input should be expanded to accept numeric input as well (basically anything that could create a proper Date should be acceptable). Its functionality is simple, useful and relies on methods that should be familiar enough to developers to understand without having to do anything particularly complex.

@Chalarangelo Chalarangelo mentioned this issue Jan 13, 2018
16 tasks
@kingdavidmartins
Copy link
Contributor

Hey @skatcat31 Thanks for bringing up this issue!!!

@Chalarangelo I totally believe if we are not careful things can get really sloppy, annoying 😫, and troublesome 😭 really fast.

If we are to update and add a lot of other date snippets we need to do so carefully, using native date methods which already follow the intended native date format. So we don't have to worry and/or think too much about it

String value representing a date. The string should be in a format recognized by the Date.parse() method (IETF-compliant RFC 2822 timestamps and also a version of ISO8601).

So it can also work with the other 20 ~ 30 or so Date.prototype.methods() which we can mix & match to implement intended snippets that fit w.e. use case

I definitely think all but toColonTime should be scrapped off all because it relies on a native method and should be called something like digitalClock24 since it a 24 HR clock. Which can also be implemented like so

const getColonTimeFromDate = _ => new Date().toTimeString().slice(0, 8);

Which returns

getColonTimeFromDate() // "13:07:00"

Which also looks like a digitalClock

I believe a good rule of thumb should be if it doesn't use native Date() with minute addition/subtractions it should be implemented

@skatcat31
Copy link
Contributor Author

I'm of the feeling that if we're going to do anything with dates we expect something similar to an ISO8601 date string for the parts we want so that no matter what library they got the string from, our behavior is consistent throughout all of our snippets.

@skatcat31
Copy link
Contributor Author

As for #536 I believe it should be addressed after this conversation

@kingdavidmartins
Copy link
Contributor

@skatcat31 Yup Yup Agreed!

I'm of the feeling that if we're going to do anything with dates we expect something similar to an ISO8601 date string for the parts we want so that no matter what library they got the string from, our behavior is consistent throughout all of our snippets.

The question now become's the following. How much and/or what exactly is allowed/expected?

Should potential contributors do the following adhering only to Date() methods when it comes date manipulation/formation? Like the following

const foo = () => {
  let time = new Date();
  return `${time.getHours()}:${time.getMinutes()}:${time.getSeconds()}`;
}
foo() // '3:12:32'

or allow users to manipulate the Date() instances at will with whatever methods they so choose?

Like using the following methods and/or more to parse date() -> 2018-01-15T08:09:34.062Z

.split()
.join()
.slice()
.map()
.splice()

Or??

@Emgo-Dev
Copy link
Contributor

Emgo-Dev commented Jan 15, 2018

Wondering if as the contributor I can give my ¢2, I'm against using the String.slice() method shown above. Merely submitted it for brevity. Though this repo values that, it doesn't serve the other purpose of explaining anything as using Date[.getHour(), .getMinute(), .getSecond()] would to construct the string.

As for using an ISO string, I'm in aggreance.

@skatcat31
Copy link
Contributor Author

Is method de-referencing part of ES6 or ES2017?

@kingdavidmartins
Copy link
Contributor

@skatcat31 Can you explain a bit more what you meant when you asked the following?

Is method de-referencing part of ES6 or ES2017?

In general? pertaining to Objects in memory? or ...?

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 17, 2018

@kingdavidmartins sorry was confusing a spec with something that hasn't been released or even made it to stage 0 yet:

Example as last I saw it:

const test = () => 'a'
const getHours = () => 'b'
var dat = new Date()

dat[ getHours(), getMinutes(), getSeconds(), test()] // [hours, minutes, seconds, 'a']
dat{ hour: getHours(), minutes: getMinutes(), seconds: getSeconds() } // {hours: hours, minutes: minutes, seconds: seconds }
[getHours()] // ['b']

de-referencing methods was a suggestion to take an instance and return something that when a method was called, was checked against the referenced object before scoping. IIRC it was still not a staged proposal( no sponsor ), hence why I asked

Turns out it's just still my confusion as it hasn't gotten a sponsor yet

@Emgo-Dev
Copy link
Contributor

Sorry @skatcat31 I had no idea that was a thing. I only wrote it that way to keep it shorter.

@skatcat31
Copy link
Contributor Author

All good. I wouldn't expect to many people to know it was a thing. I'm pretty certain I'm the only one who reads the mailing list publications and TC39 in this group

@kingdavidmartins
Copy link
Contributor

@skatcat31 ahhh gotcha 😆
Jesus not even stage 0 yet? 😖 I barely checkout Stage 2 ~ 3 as it is 😨

@kingdavidmartins
Copy link
Contributor

kingdavidmartins commented Jan 17, 2018

Just to confirm @skatcat31, what is your take on the following?

The question now become's the following. How much and/or what exactly is allowed/expected?

Should potential contributors do the following adhering only to Date() methods when it comes date manipulation/formation? Like the following

const foo = () => {
  let time = new Date();
  return `${time.getHours()}:${time.getMinutes()}:${time.getSeconds()}`;
}
foo() // '3:12:32'

or allow users to manipulate the Date() instances at will with whatever methods they so choose?

Like using the following methods and/or more to parse date() -> 2018-01-15T08:09:34.062Z

.split()
.join()
.slice()
.map()
.splice()
Or??

So we can get #536 sorted

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 17, 2018

@kingdavidmartins pretty sure I said if we do any Date related things they should be to ISO8601 compatible strings to the extent that we need, not Date objects.

For reference the following produces an ISO8601 compatible ms string

new Date().toISOString() // we could use this for examples

Now our methods are purely string related and have an exact time to work with

Reasoning

Date libraries that adhere to ISO8601 would also adhere to the fact that without a timezone on the end of the string it is parsed in local time, the exact thing that caused us to remove toEnglishDate and the likes. By enforcing a String and not an instance we make it not matter the input, it won't change due to parsing.

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 17, 2018

Addendum, if we only need parts of the ISO8601 string, the method should NOT expect a full one, just enough of one( as per ISO8601, partial ISO8601 are valid )

const toEnglishDate = isoString =>
  (isoString = isoString.split(/\-|T/)) && [isoString[1], isoString[2], isoString[0]].join('/')
toEnglishDate('2018-01-02') // '01/02/2018'

@skatcat31
Copy link
Contributor Author

... As a sidenote I think we can redo toEnglishDate now with this version XD

@skatcat31
Copy link
Contributor Author

I'm at a loss for a way to put this into the contribution guidelines without sounding stupid

@kingdavidmartins
Copy link
Contributor

@skatcat31 I believe we are in agreement on adhering to ISO8601 & partial ISO8601

However were I believe we disagree on is the process or rather the proposed structure at which we arrive at our conclusion.

My question is why go about is the following way?

const toEnglishDate = isoString =>
  (isoString = isoString.split(/\-|T/)) && [isoString[1], isoString[2], isoString[0]].join('/')
toEnglishDate('2018-01-02') // '01/02/2018'

When it can also be accomplished the following

const foo = () => {
  let time = new Date();
  return `${time.getMonth()+1}/${time.getDate()}/${time.getFullYear()}`;
}
foo() // '1/18/2018'

Isn't our objective to ensure that the returned value adheres to ISO8601 & partial ISO8601 which really comes down to how we format the output?

Also since using the new Date() constructor creates a JavaScript Date object / instance for the current date and time according to system settings. Can't we just used the provided methods of the Date() objects to grab what's needed and format according to follow ISO8601 & partial ISO8601. Rather than using .toISOString() to convert it to a string just so we can now parse it with regex only to use .join() to format it in this example. Which seems a little overkill/unnecessary?

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 19, 2018

@kingdavidmartins like I stated before:

Date libraries that adhere to ISO8601 would also adhere to the fact that without a timezone on the end of the string it is parsed in local time, the exact thing that caused us to remove toEnglishDate and the likes. By enforcing a String and not an instance we make it not matter the input, it won't change due to parsing.

The method mentioned was that when passed to Date, ISO8601 strings without a timezone are parsed 'in local time', leading to confusing results for anyone who has no idea of the inner workings of ISO8601. By making it use a string absolutely, we alliviate that error in that '2018-01-01' will always be '01/01/2018' instead of '12/31/2017' or the current time only( the problem we experienced with toEnglishDate, and what would happen if we use new Date() )

foo() // a time
// adjust clock in system settings
foo() // a DIFFERENT time

vs

var d = new Date().toISOString()
toEnglishDate(d) // a time
// adjust system clock
toEnglishDate(d) // the SAME time

The top only has the use case of "Current time in English Date Format" the bottom has the use case of "Format an ISO8601 time in English Date Format", which also covers the previous case

@Emgo-Dev
Copy link
Contributor

Emgo-Dev commented Jan 19, 2018

I think I get it. If a user unknowingly passes in an improper ISO8601 string the Date instance they use from whatever API will give them an unexpected result instead of the function acting on that string directly. And by expecting a string according to the ISO8601 rather than the result of the Date API, we can expect a consistent format?

This makes it unfortunate that you need to parse this string in every function unless this repository adopts a functional approach allowing other snippet functions to be nested in higher order functions.

I'm not sure whether use of standard functions & methods has changed in contribution, but is Date not considered?

Both methods (@skatcat31, @kingdavidmartins) encourage the understanding of ISO8601, either by explicitly showing how to process the string, or by requiring a proper input.

@kingdavidmartins
Copy link
Contributor

@skatcat31 Although I understand the reasoning behind what your saying, what I'm saying is also the following. I don't think its practical and can't see how's it's confusing

If I live in NY and someone else live in Spain, or in another time zone let's say. Shouldn't we see different time's? shouldn't we see the time on our machines? And see our local time with regards to our timezone?

Also although by using toISOString() we'll be essentially seeing the "same time" can't there still be confusion? For example in NY it is currently Jan 18 2018 8:37 pm however new Date().toISOString() return the following 2018-01-19T01:37:00.399Z which if I am not aware can also confuse me since I'm expecting to see my local time? assuming I don't know we are using a zero UTC offset

@kingdavidmartins
Copy link
Contributor

kingdavidmartins commented Jan 19, 2018

@skatcat31 I believe it really comes down to the assumption of how our user/users will use Time/Date related snippets and what we believe their expected return should be

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 19, 2018

@kingdavidmartins

var a = new Date('2018-01-01')
var b = time =>
  `${time.getMonth()+1}/${time.getDate()}/${time.getFullYear()}`
b(a) // "12/31/2017" because I'm PT(-800/-700)

The above is why I say we should avoid the Date libraries. Pointed out to us by @fejes713 when toEnglishDate was around

// convert times on comments into Dates for display
comments.map(copySimple).map(inWith('date',toEnglishDate)).forEach(attachToPost)

This very common use case is why toEnglishDate shouldn't only display the current time, but instead take any compatible input

toEnglishDate(new Date().toISOString())
toEnglishDate(new Moment().toISOString())
toEnglishDate(new Timeless().toISOString())

achieves the same as your code snippet, and is able to be implemented with several libraries, or a string from a data source

General, re-useable code is always better than specific use case code if there is a chance it can be used in more than one place IMO

In regards to seeing different timezones:

new Date('2018-01-01T00:00:00').toISOString() != new Date('2018-01-01T00:00:00Z').toISOString()

While that can be desired, it can also be greatly confusing. A good example was the 2 weeks FB comments were displayed in local time, leading to comments saying they came from the past. People where very confused

@skatcat31
Copy link
Contributor Author

Possible compromise in functionality(not code)

const toEnglishDate = (isoString = new Date().toISOString()) =>
  (isoString = isoString.split(/\-|T/)) && [isoString[1], isoString[2], isoString[0]].join('/')
toEnglishDate() //display the current English Date
toEnglishDate('2018-01-01') // 01/01/2018 regardless of timezone

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 19, 2018

@Emgo-Dev pretty close. However parsing ONLY needs to be done to the extent our snippet needs of the string. More complex regex could be used for generating different functions and it gets a little confusing and it would be required every time, but it also means each snippet is completely standalone.

As for Date, toEnglishDate used to be the b method above(functionally) which as you can see lead to the timezone discrepancies that have led to this discussion

@kingdavidmartins
Copy link
Contributor

kingdavidmartins commented Jan 23, 2018

@skatcat31 I believe you are onto something.

having dateMethods accepting date strings from different libraries can and will deff make our methods more flexible

I'm proposing the following format

const dateSnippet = (str, ISO = false) => {
  // Code
}

An example being the following

const tomorrow = (dateStr, ISO = false) => {
  let t = dateStr === undefined || typeof(dateStr) === 'boolean' ? new Date() : new Date(dateStr);
  t.setDate(t.getDate() + 1);
  let month = `${t.getMonth()+1}`.padStart(2, '0');
  let day = `${t.getDate()}`.padStart(2, '0');
  let isTomorrow = `${t.getFullYear()}-${month}-${day}`;
  
  if (typeof(ISO) === 'boolean' && ISO === false) {
    return isTomorrow;
  } else {
    let ISOTomorrow = new Date(isTomorrow).toISOString()
    return (isoT = ISOTomorrow.split(/\-|T/)) && [isoT[0], isoT[1], isoT[2]].join('-')
  }
};

Resulting with the following

tomorrow('Sun Dec 31 2028') // 2029-01-01
tomorrow('Feb 28 2028') // 2028-02-29
tomorrow('2018-05-3') // 2018-05-04
tomorrow('2018-01-01T00:00:00') // 2018-01-02
tomorrow('Wed Oct 05 2011 16:48:00 GMT+0200 (CEST)') // 2011-10-06
tomorrow('Wed Oct 05 2011 00:48:00 GMT+0200 (CEST)', true) // 2011-10-05

This way we can always default to local time & then pass in true or anything else as a second arg to get the ISO formatted using the zero UTC offset

*Note: *The str is expected to accept ISO8601 & partial ISO8601. When ISO is set to false I mean not using .toISOString() which means we won't using the zero UTC offset. Until set to true

@Chalarangelo
Copy link
Owner

Honestly, @kingdavidmartins, the overhead is too long for this kind of thing. I mean we need to figure this out somehow, but it's gonna confuse readers a lot and make us tag all our date snippets advanced. In general, I think we should just stick with a few very simple date methods and leave the rest to libraries like moment. For learning purposes, a few basic Date manipulation functions are more than enough.

@skatcat31
Copy link
Contributor Author

@kingdavidmartins sadly I agree with @Chalarangelo about the overhead. It's much easier to reason that we're restricting them to an ISO8601 compatible string for us to manipulate, even if it means we have regex in place to parse the string. It leads to completely compatible implimentations with the only knowledge being "Takes a string like new Date.toISOString()" versus "Uses the Date library to parse strings and optionally supply a UTC marked backing" which has timing restrictions which a string does not.

@Emgo-Dev
Copy link
Contributor

Emgo-Dev commented Jan 24, 2018

I think we're beginning to approach two separate concerns. Interpreters, which @kingdavidmartins example behaves like, and formatters.

Interpreters would provided good utility expanding Date default understanding of human readable date strings.

But Formatters should return a formatted string from a single expected standard.

I played around more with this idea of relying on regex and only an ISO format. I think it fits best for the repo.

const getIsoDate = isoString => isoString.match(/\w{4}-\w{2}-\w{2}/)[0]
const getIsoYear = isoString => isoString.match(/^\w{4}/)[0]
const getIsoMonth = isoString => isoString.match(/\w{2}(?=-\w{2}T)/)[0]
const getIsoDay = isoString => isoString.match(/\w{2}(?=T)/)[0]
const getIsoTime = isoString => isoString.match(/\w{2}:\w{2}:\w{2}/)[0]
const getIsoHour = isoString => isoString.match(/\w{2}(?=:)/)[0]
const getIsoMinute = isoString => isoString.match(/\w{2}(?=:\w{2}\.)/)[0]
const getIsoSecond = isoString => isoString.search(/\w{2}(?=\.)/)[0]

The only problem might be how specific they are. They'd be divided among many individual snippet files. They do what we've discussed but the regex becomes redundant.

You could achieve a tomorrow result using the above like this.

let date = new Date("January 15 2015 12:12:12").toISOString();
let year = getIsoYear(date)
let month = getIsoMonth(date)
let day = getIsoDay(date)
let tomorrow = `${year}-${month}-${day[0].concat(parseInt(day[1]) + 1)}`

@Chalarangelo
Copy link
Owner

Having snippets with prerequisites is something we have discussed before and we have decided that we will never do this, as it goes against the core idea of the project. Every snippet has to be self-contained and understandalbe without prerequisites. I suppose that the only way to make this workable is to tag most of the date snippets advanced right from the start or something similar and just have quite a lof of overhead which we cannot easily avoid.

@skatcat31
Copy link
Contributor Author

skatcat31 commented Jan 24, 2018

I feel like at this point we've reached the conclusion: Keeping them in the main repo exposes us to a lot of confusing design patterns or hard to rely on libraries due to time differences.

Because of this I'm of the opinion that we should not have Date or DateString formatting functions in the main repository. Due to an expanded required knowledge they are either too advanced, or too confusing unless you know the Date libraries or Regex or ISO8601. To this day many advanced and high level developers STILL have problems with Date logic

@Chalarangelo
Copy link
Owner

So, in regards to existing snippets, should we keep the 3 date snippets we have? They seem usable and don't seem to cause a lot of trouble as they are, as formatDuration is a simple mathematical transformation, getDaysDiffBetweenDates accepts a Date object, which should be safe to work with and tomorrow is also relatively safe, based on the fact it requires no input from the user.

In regards to #536, I suppose that getColonTimeFromDate is safe to use, based on the fact that it accepts a Date object (so we do not have to deal with anything there) and getMeridiemSuffixOfInteger is also safe due to the fact that it's a simple mathematical transformation.

@Chalarangelo
Copy link
Owner

After re-reading the whole thread, I think I have a suggestion about the date category as a whole, one that could go into the guidelines:

Date snippets considerations

As far as snippets that deal with date parsing and formatting are concerned:

  • Snippets that apply mathematical transformations to raw millisecond (ms) values, such as formatDuration are acceptable and considered relevant to dates.
  • Snippets that apply string transformations to raw ms values are considered relevant to dates.
  • Snippets that accept Date objects and use the prototype's built-in functionality or apply transformations are acceptable, as long as they do not have confusing edge-cases.
  • Snippets that parse Date objects internally from strings or numeric values are going to be declined, as they are going to have edge-cases that need to be dealt with.

I think the above rules should cover it all and keep everything safe and consistent, due to the fact that dealing with existing Date objects lets the developers tackle the issue of localization on their own and, for snippets that use raw ms and string data, we only apply transformations without parsing dates in them. That being said, all the currently accepted snippets in the date category are safe to use (and I see no reason to remove them as they are not really all that problematic and don't seem to have any unhandled edge-cases), while the snippets in #536 seem to conform to these guidelines, so they are safe to merge and showcase to new users.

Opinions? @kingdavidmartins @fejes713 @skatcat31

@lock
Copy link

lock bot commented Dec 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants