search

atom / node-oniguruma

Oniguruma Node Module
http://atom.github.io/node-oniguruma
MIT License
121 stars 46 forks source link

Change the for statement of OnigRegExp.prototype.captureIndicesForMatch to map #83

Closed abetomo closed 6 years ago

abetomo commented 6 years ago

It is a point where we lost assignment in var, which is likely to cause side effects.

In addition I changed the null judgment. This made the indent shallow.

lee-dohm commented 6 years ago

Thanks for the contribution here. I'm not sure what the benefit is here? Can you give some more detail as to what this improves?

abetomo commented 6 years ago

@lee-dohm Thank you for review. It is a point where we lost assignment in var, which is likely to cause side effects.

In addition I changed the null judgment. This made the indent shallow.

lee-dohm commented 6 years ago

This project is fairly high-risk code to change. Unfortunately, we're loathe to change the code that, for all intents and purposes, appears to be working for something that might occur. If you can create a test case that shows that the original code does break and under what circumstances and then show how this new code fixes it, we'll take another look.

Thanks again for the contribution!

abetomo commented 6 years ago

Thank you for review! There is no particular problem before the change. It was a suggestion to change to a code with fewer side effects.

Because it is not because there was something wrong, I will close it.