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

Remove an unused .trim() method #11

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@drify
Copy link

drify commented Feb 3, 2019

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

This comment has been minimized.

Copy link
Author

drify commented Feb 3, 2019

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

@mhchem

This comment has been minimized.

Copy link
Owner

mhchem commented Feb 3, 2019

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

This comment has been minimized.

Copy link
Owner

mhchem commented Feb 4, 2019

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

@mhchem mhchem closed this Feb 4, 2019

@drify drify deleted the drify:patch-1 branch Feb 4, 2019

@drify

This comment has been minimized.

Copy link
Author

drify commented Feb 4, 2019

Thank you!
(though I still wish to be a contributor😆)

@mhchem

This comment has been minimized.

Copy link
Owner

mhchem commented Feb 4, 2019

(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.)

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