[CLOSED] Simplify FindUtils.parseDollars

[core-ai-bot]

Issue by MarcelGerber
Saturday Apr 19, 2014 at 23:22 GMT
Originally opened as adobe/brackets#7582

---

I tried to simplify/comment the quite complicated FindReplace helper function parseDollars, but to be honest, I don't know if I made it better or worse.

Let's see if someone understands the function now 😆

The only architectural change is that parseInt is only called after we made sure index is indeed an integer (and not &).

---

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/7582/commits

[core-ai-bot]

Comment by dangoor
Monday Apr 21, 2014 at 17:39 GMT

---

@njx

[core-ai-bot]

Comment by njx
Thursday Apr 24, 2014 at 00:11 GMT

---

Marking low priority. (@SAplayer - not intended to be an insult :), but we're starting to try to sort out pull requests that need to be dealt with more urgently from ones that can wait a bit.)

[core-ai-bot]

Comment by dangoor
Tuesday Jun 17, 2014 at 13:50 GMT

---

Stealing this from@njx.@SAPlayer the change seems fine. Would you be willing to add some test code for this to FindReplace-test.js? I realize there are no tests there now, but it would be nice to have some that demonstrate what this function does (and that it actually does it!)

Thanks!

[core-ai-bot]

Comment by njx
Tuesday Jun 17, 2014 at 14:58 GMT

---

Before merging this, let me check whether it will merge cleanly with the replace-across-files branch - I might have moved the function elsewhere.

[core-ai-bot]

Comment by dangoor
Tuesday Jun 17, 2014 at 15:10 GMT

---

@njx OK. I was surprised it hadn't moved already :)

Getting replace-across-files in is higher priority, so you can just carry on, merge that and then we'll see what needs to happen.

[core-ai-bot]

Comment by njx
Tuesday Jun 17, 2014 at 21:07 GMT

---

Yup, it's moved to FindUtils.js in my branch. Should be easy enough to copy it there after that lands and put up a new PR. (Sorry, I should have landed this before starting the branch in the first place.)

[core-ai-bot]

Comment by MarcelGerber
Tuesday Jun 17, 2014 at 22:46 GMT

---

@dangoor There are no tests specificially for this function, but there are many for the functionality:
https://github.com/adobe/brackets/blob/master/test/spec/FindReplace-test.js#L1173-1331

[core-ai-bot]

Comment by dangoor
Wednesday Jun 18, 2014 at 13:57 GMT

---

@SAPlayer Ahh, you're right. That's fine.

[core-ai-bot]

Comment by MarcelGerber
Friday Jun 20, 2014 at 00:42 GMT

---

@dangoor@njx Merged and ready.

[core-ai-bot]

Comment by dangoor
Wednesday Jun 25, 2014 at 20:37 GMT

---

@SAPlayer Thanks for merging this in with the recent find changes.