search

gr2m / moment-parseformat

A moment.js plugin to extract the format of a date/time string
http://gr2m.github.io/moment-parseformat
Apache License 2.0
45 stars 30 forks source link

Several fixes, new formats, and tons of tests #90

Closed hillmanov closed 3 years ago

hillmanov commented 5 years ago

First off, thank you for this library! It has served our needs well for some time, but recently our needs have changed. We needed support for a LOT of different and odd date formats that were not originally caught by this library, so I made the changes necessary to support our needs. The vast majority of the tests were generated programmatically, which is why there are so many.

During the process some of the open issues were addressed, so they are included in this PR as well.

If the changes to the code or the tests are not inline with what you have in mind, that is just fine. My organization will continue to use our fork unless this gets merged in.

Let me know how you would like to proceed!

gr2m commented 5 years ago

Wow thanks Scott! I know you put a lot time into this, but because it’s such a big PR, it will take lots of time for me to review, time I don’t have right now due to family.

Can you please split it up? Otherwise I fear this will get blocked for a long time, it would be better to get some things merged we agree on and have separate PRs for things that we can discuss, without blocking everything.

hillmanov commented 5 years ago

@gr2m

I completely understand - t is a very large PR! The vast majority of the changes are in the tests, with a pretty good handful of new regular expressions and format = format.replace... statements.

I am not worried personally about the timeline of this getting in, and if it takes a long time to get to that is just fine for me.

As far as breaking it up, I know and understand that would be ideal, however, that would prove to be pretty difficult to do. I knew which formats I had to support coming in so I created almost all of the tests at that point (over 1,200 failed first run!) and I just churned until they all passed. A fix for one section would break another so in the end it felt like an 'all or nothing' kind of PR. This isn't how I like to develop software, but we needed support for these formats pretty fast so the rest of the project could continue.

I will see what I have time for myself since now that it is working the way we need, I can move on with my project.

RichardCr commented 3 years ago

@gr2m Hi there! Any chance this can be merged in? This fixes several issues for me. (07 Dec 20 -> without this gets DD MMM D)

gr2m commented 3 years ago

@RichardCr I've reviewed the PR in full now. I can go ahead and merge it and then do follow up comments to address my comments, but I want to give @hillmanov a week to address them first

hillmanov commented 3 years ago

@gr2m and @RichardCr I'll see what I can do. I no longer work at the company I was at when I made this PR, so I don't have the ability to push stuff to that forked repo anymore. However, I am still close with several of the devs there, so I am hopeful that I can get something done.

RichardCr commented 3 years ago

@gr2m @hillmanov you guys are amazing :) Looking forward to it. It is a very useful library.

RichardCr commented 3 years ago

@gr2m Hi there, just seeing if it is likely this will get merged or not. Thanks!

gr2m commented 3 years ago

@gr2m Hi there, just seeing if it is likely this will get merged or not. Thanks!

can you help resolve the conflicts?

hillmanov commented 3 years ago

@gr2m conflicts resolved (finally) and all tests pass. I updated many of the test cases to get them compliant with the standard formatting requirement as well.

Not sure what is going on with the failing travis-ci build though - Node 6 is just failing to install some things, but it doesn't look like it has anything to do with this PR.

I also see that the code coverage percent went down, but this PR also increases the amount of test by... a lot.

RichardCr commented 3 years ago

@gr2m Hi there, just seeing if it is likely this will get merged or not. Thanks!

can you help resolve the conflicts?

I'm still pretty new to all this, I wouldn't rate myself not to break something. Happy to try though. Although it seems like it is already done! :1st_place_medal:

hillmanov commented 3 years ago

@RichardCr I was finally able to clean my PR up as far as the conflicts are concerned. There looks to be some kind of issue with the CI checks with Node 6 - something isn't running successfully, but it doesn't look like it is related to this PR. I think we just need to wait to hear from @gr2m now.

RichardCr commented 3 years ago

@hillmanov Thanks so much. I really appreciate everyone who has contributed to this!

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 3.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

gr2m commented 3 years ago

It looks like we accidentally introduced a breaking change: https://github.com/gr2m/moment-parseformat/issues/109.

I'll revert these changes and publish 3.1.1, then release a breaking version 4.0.0 with the changes here

gr2m commented 3 years ago

now released as v4.0.0