Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Set of always wish had utility functions #342

Merged
merged 21 commits into from Feb 11, 2012

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Feb 9, 2012

So this change implements set of utility functions that are super useful every now and then, but I always either was implementing them in place or tried to avoid them all together. I have faced the same thing once again when working on new events #312 and finally I have decided to put these together. Also since it does not really relates to the events work I cherry-picked these changes into separate (this) branch.

So this adds some utility functions and also normalizes Enqueued into defer to match backbone. Still I keep Enqueued with a deprecation warnings.

// Exporting `enqueue` alias as `defer` may conflict with promises.
exports.remit = defer;
exports.Enqueued = function Enqueued() {
console.warn('Function was renamed to `defer` please use it instead.')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we print stack trace on warnings, so it would be usefull to give more information.

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 don't think we print stack trace on warnings, so it would be usefull to give more information.

I think stack trace would add too much noise here. What about just 'Enqueue' function was renamed to 'defer' please use it instead.' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure!

assert.ok(delayed, 'delayed the function');
done();
}, 150);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is many chances that this test will have intermitent failures on slow hardware.

You may take a look at setTimeout unittests:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/tests/test-timer.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true, order of timeout calls will be adequate of the timer ms regardless of soft / fast hardware. And that's only thing we test here.

@ochameau
Copy link
Contributor

I don't think it is relevant to use the metadata until the feature is landed.

In term of code, it looks good, just some minor stuff to check.

But I have some more general concerns:

  • You took code from underscore. They are using MPL licence, so it should be OK, but I think that we have to mention authors or import their licence header/file ?
  • Why not using more underscore API? May be the whole underscore code?
  • You basically tend to import functional languages pattern. I won't argue that it is functional pattern is bad. (ochameau = ocaml = functional language ...) But JS is not known to be a functional language and for now, doesn't contains important pieces of functional languages, like a syntax partial application and composition. So that we can simulate these features by creating anonymous functions on the fly. Doing this has pros and cons, it allows to use shorter functional patterns and have a different code layout based on functions instead of prototype, dynamic dispatch, modules, objects, classes whatever ... But it can end up being harder to debug as stack traces will be polluted with these helper functions. And it is harder to catch an error when you end up causing an exception in one of these helper method. Like giving a wrong argument to compose, curry. There isn't any type checking, whereas if it was supported by the language, it would throw an explicit message.

Having said that it doesn't make sense to block this pull request as it fits into our review/code guideline, so I'm r+ it (with licence/ownership issue being addressed). But I'd like to engage a discussion during our weekly meeting about such choice.

@ochameau
Copy link
Contributor

Oh and if you land it, can you stash the history before landing it.
It doesn't sounds valuable to keep all these intermediates revision. It ends up polluting repository log a lot!

@Gozala
Copy link
Contributor Author

Gozala commented Feb 11, 2012

I don't think it is relevant to use the metadata until the feature is landed.

I'll remove it for now

You took code from underscore.

Nope I have implement their APIs, also as matter of fact there are slight differences as well. On the other hand I did took their examples.

Why not using more underscore API? May be the whole underscore code?

I personally have not had a need for that, but I'm open to that if you think it would make more sense.

You basically tend to import functional languages pattern.

I'm trying to improve code readability by reducing unnecessary noise from the code, and eliminating patterns used all over the place. I do agree that code may get little bit harder to debug, but on the other hand this makes it much less code to debug. Also composition has major testability advantage since if you're function a is tested to work correctly and function b is tested to work correctly then you guaranteed that composition of a and b will work. In other words I do think advantages out-weight disadvantages here and improving stack traces should be addressed separately (probably by devtools team).

@Gozala
Copy link
Contributor Author

Gozala commented Feb 11, 2012

So

  1. I have addressed all the review comments I believe.
  2. Regarding licensing, we just implement APIs defined by underscore and we don't use underscore itself, there for I have added disclaimer with a credit to Jeremy of underscore.
  3. Regarding stashing a changes. We discussed this on IRC, but I'd like to stress that I'm against this as it throws away history (that maybe annoying until needed but can be easily suppressed via git log master --merges), but when needed it's very powerful with combination of git blame / bisect (which was helpful in detection of 1.4.3 bug introduced by simple-prefs).

@Gozala
Copy link
Contributor Author

Gozala commented Feb 11, 2012

@ochameau before landing this I'd like to know:

  1. If we should rename api-utils/utils/function -> api-utils/functional ?
  2. If 1 is true should we keep api-utils/utils/function for some time with deprecation warnings, or just remove it ?

Gozala added a commit that referenced this pull request Feb 11, 2012
Set of always wish had utility functions r=@ochameau
@Gozala Gozala merged commit 03d0556 into mozilla:master Feb 11, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants