mhchem / MathJax-mhchem

3rd-party extension to MathJax for typesetting chemical equations
Apache License 2.0
95 stars 13 forks source link

Remove an unused .trim() method #11

Closed drify closed 5 years ago

drify commented 5 years ago

I watch the changes you made to fix #10 , and appreciate your prompt response. However, this line of code seems to have a problem. #L1193

m[3] = m[3].replace(/^\s|\s$/, m[3]);  // .trim();

The code wouldn't work as intended for sure. A more correct .trim() should be:

m[3] = m[3].replace(/^\s+|\s+$/g, '');

The replacement should be an empty string, rather than the original string, which was like a typo or something. Also notice the g flag; without that, only the first space would be replaced.

However, I tried some tests and found this bug didn't affect the displayed result, so I read the code more carefully. The corresponding part of regex should be at #L236 :

/...([eE]|\s*(\*|x|\\times|\u00D7)\s*10\^).../

These are the fourth and fifth capturing groups, which are converted to m[3] and m[4]. The only occasion when m[3] contains spaces is [eE] mismatches, but then m[4] must match, and as we have

m[3] = m[4] || m[3];

we can guarantee that m[3] will not contain any whitespace characters, so .trim() is unnecessary.

drify commented 5 years ago

By the way, what's the purpose of .slice(1, 99)? I think .slice(1) should work fine...

mhchem commented 5 years ago

Oh, how embarrassing! Phew, this is unused code. Thanks a lot for your analysis.

Yes, .slice(1) works as well. Yesterday, I realized that IE8 produced wrong results for some inputs. I figured that this was caused by a .splice(1), because the 2nd parameter is not optional in IE8. So it became .splice(1, 99) and later on, I changed it to a .slice(1, 99), missing the fact that a simple .slice(1) would do as sell.

mhchem commented 5 years ago

I used the chance to tidy up a little more. ad06527b5c22aad68a6e2294dbbb461792b3996c Thanks again.

drify commented 5 years ago

Thank you! (though I still wish to be a contributoršŸ˜†)

mhchem commented 5 years ago

(Sorry. But even if I had merged your pull request, your would not have been listed on https://github.com/mhchem/MathJax-mhchem/graphs/contributors.)