rewrote all array prototype stuff to specific ROT.RNG methods

[psvensson]

Hi again! I took the time to remove the array prototypes that interfered and moved them into the ROT.RNG module instead.

The tests pass nicely - thank you so much for a very thorough test-suite! ^_^

[ondras]

I am sorry for the time you wasted on this, but I am not going to accept this patch. For two reasons:

1) see https://github.com/ondras/rot.js/issues/83 for a more thorough discussion about why this adjustment is really not necessary,

2) you are introducing a compatibility-breaking change. There is quite some amount of rot.js-using code in the wild that relies on these prototypal enhancements; people are expecting this to work even if they update to a more recent rot.js version. We would be messing with a frozen API, something that is frowned upon hard.

[psvensson]

I understand this, after your previous comment.

But at least I am happy and have a subsystem that does not interfere with anything else while doing what it should. If you get any other comments from people who might look for a similar solution, please feel free to direct them to me.

On Sat, Jan 2, 2016 at 10:04 PM, Ondřej Žára notifications@github.com wrote:

Closed #84 https://github.com/ondras/rot.js/pull/84.

— Reply to this email directly or view it on GitHub https://github.com/ondras/rot.js/pull/84#event-504044892.

[unstoppablecarl]

Why not put this in a 1.0 branch?

[ondras]

Why not put this in a 1.0 branch?

Because I like the API as-is. Having the [a, b, c].random() is incredibly convenient. I intend to remove these methods only if they get standardized and replaced by a different official implementation.

[unstoppablecarl]

It is accepted best practice to not modify core object prototypes. I have had problems with this as well and I know of at least 2 people that have had to modify rot.js to fix problems because of the methods added to the array prototype.

If you like adding methods to the Array prototype you should do that in project code, not in lib code intended to be used as a dependency by others.

Many js beginners use rot.js when learning js. It is doing them a disservice to have such poor practices in source code they are likely to learn from.

This is a trivial change that users of the lib should not have to undo themselves with their own branch of rot.

[ondras]

It is accepted best practice to not modify core object prototypes.

True. But the reason for this is to prevent naming collisions, not to clash with poor enumeration code. And I am willing to undertake the naming collision risk, as stated before.

(Note that this applies to Number.prototype.mod and String.prototype.{l,r}pad as well.)

I have had problems with this as well and I know of at least 2 people that have had to modify rot.js to fix problems because of the methods added to the array prototype.

Then perhaps you and those 2 people should have submitted an issue and discussed this problem? Fixing rot.js is a very poor solution and I am certain that there would be a better way to solve it.

Also, the fact that a solution was applied does not imply that the solution was good or that other solutions could not be better.

If you like adding methods to the Array prototype you should do that in project code, not in lib code intended to be used as a dependency by others.

Does this apply to polyfills for standard methods as well? Because these are regularly added to library code and noone is complaining. A logical consequence of this behavior is that the for p in arr iteration would be broken anyway. Iterating correctly is the way to go here.

Many js beginners use rot.js when learning js. It is doing them a disservice to have such poor practices in source code they are likely to learn from.

And my argumentation would be:

1. beginners use rot.js when learning js
2. I am fully aware of risks related to primordial prototype enhacements
3. with the knowledge of what and when might happen, I am pretty happy with the way these prototypes are enhanced
4. therefore, I am happy with how the code looks
5. therefore, I am happy if others write their code in the same manner
6. therefore, I am happy when beginners learn to follow this example (provided they understand what is really happening and going on under the hood)

This is a trivial change

The change, primarily, breaks backwards compatibility.

that users of the lib should not have to undo themselves with their own branch of rot.

Very true! They should fix their own code instead.

Please show me one particular example of a code that requires removing these enhacements from rot.js (and there is no other better way around).

[unstoppablecarl]

Polyfills have a specified signature that should never change. If there is collision the behavior should be the same.

Example: Using a lib that adds a Array.prototype.random method with a different signature or behavior than the rot.js implementation resulting in a collision.

With no standard specification for Array.prototype.random neither implementation can objectively argue to be more "correct", but one of them must be renamed and references refactored to avoid the collision.

I think the best solution to this problem would be to make a trivial change to the lib preventing this from being possible at all.

If you are pushing a point release breaking changes are inferred and previous versions remain accessible via npm.

[toomuchpete]

:+1: To not catering to folks who want to enumerate improperly.

[ondras]

With no standard specification for Array.prototype.random neither implementation can objectively argue to be more "correct", but one of them must be renamed and references refactored to avoid the collision.

To quote myself from the #83:

As soon as the random or randomize methods get standardized, I will immediately remove my own variants from Array.prototype (as long as they differ from the official version)

The same also applies to the hypothetical scenario of another library implementing Array.prototype.random with a different signature and/or semantics.

Now the fun part is that rot.js is now almost four years old -- and yet I have never encountered a single naming collision with respect to primordial prototype enhancements.

I think the best solution to this problem would be to make a trivial change to the lib preventing this from being possible at all.

I think there really is no problem so far. Let's seek problem solutions aftery the problems arrive.