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

Added workaround for Moment instances #31

Closed
wants to merge 1 commit into from
Closed

Added workaround for Moment instances #31

wants to merge 1 commit into from

Conversation

neemzy
Copy link

@neemzy neemzy commented Oct 11, 2016

Hi,

I noticed the lib is unable to deal with Moment instances. I therefore added a workaround to avoid processing them.

I'd understand if you don't want to merge this, as it is undesirable to handle workarounds for miscellaneous libraries, but you can get rid of it in your next major version (where you said you'd implement custom strategies, allowing such stuff to happen in userland) and allow people to use your library with Moment, the most popular date/time management library in the JS ecosystem, in the meantime.

Best regards!

@unclechu
Copy link
Owner

unclechu commented Oct 11, 2016

@neemzy sorry, but I'm not going to merge it because of:

  1. It's unwanted additional dependency;
  2. It's special case, we can't make this library be fat adding logic for every special case, it supposed to be small;
  3. Look at your changes, you mixed up spaces and tabs, there's .editorconfig file inside repo to avoid this (install editorconfig plugin to your code editor or IDE to prevent this mess in the future).

In next major release I planned to implement API for custom strategies for special cases like this one.

@unclechu unclechu closed this Oct 11, 2016
Copy link
Owner

@unclechu unclechu left a comment

Choose a reason for hiding this comment

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

Mixed spaces and tabs.
Test is very stupid, doesn't check anything.

@unclechu unclechu self-assigned this Oct 11, 2016
@neemzy
Copy link
Author

neemzy commented Oct 11, 2016

  1. The additional dependency is specified as a devDependency; it will only be installed if you run npm install on this sole repository, in order to work on it and run tests, where this is actually needed. You'll notice I avoided making use of Moment in the library code precisely in that aim.
  2. I already adressed that concern. This fix was meant to be temporary and disappear when rendered obsolete by the next major version. Of course, I respect your point of view as I explained in the PR comment, but I'm beginning to wonder if you actually read it ;)
  3. (and second comment) No need to go all lofty on me just because I skipped your project's indentation style. As for the test, it does guarantee the library works with Moment instances by preserving its methods.

If I may, you might want to be extra careful when dealing with contributors, even if you don't like their work and aren't going to merge it, which you are absolutely free to do without using that kind of tone. Will not contribute again. Cheers! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants