[CLOSED] Fix for Issue 3339

[core-ai-bot]

Issue by WebsiteDeveloper
Thursday Apr 04, 2013 at 07:52 GMT
Originally opened as adobe/brackets#3340

---

Fix #3339

---

WebsiteDeveloper included the following code: https://github.com/adobe/brackets/pull/3340/commits

[core-ai-bot]

Comment by WebsiteDeveloper
Thursday Apr 04, 2013 at 18:51 GMT

---

Also added fixes for two other cases@peterflynn mentioned.

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Apr 05, 2013 at 08:11 GMT

---

@peterflynn also added code to replace the entire token, when invoking the in hints inside a token.

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Apr 05, 2013 at 08:12 GMT

---

unit tests will be added in a separate pull after this got merged.

[core-ai-bot]

Comment by peterflynn
Monday Apr 08, 2013 at 17:26 GMT

---

We should really add unit tests here too -- both for some entity hinting cases and for the tag hinting cases that are currently broken (and which the existing tag hinting tests haven't caught).

[core-ai-bot]

Comment by peterflynn
Monday Apr 08, 2013 at 17:27 GMT

---

Oops sorry, just saw the above note about adding unit tests later. Ideally we like to do it all in one pull request, but this seems ok since we definitely need to get the fix part landed ASAP before sprint end.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Apr 08, 2013 at 17:30 GMT

---

thats what i thougth ;)

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 18:54 GMT

---

Although you haven't changed it in this pull request, I just noticed this function:

    function _encodeValue(value) {
        return (value.indexOf("#") === -1) ? value.replace("&", "&") : value.replace("&", "&").replace("#", "#");
    }

There's no need to search for "#" because replace will do nothing if it's not found:

    function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#");
    }

Question: Why is the semi-colon (;) not also encoded since it is decoded in the associated _decodeValue() function? In general, encode/decode functions should do the exact opposite of each other.

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 18:59 GMT

---

Please don't push any changes until I say "review is complete". I'm looking at the code in both the github pull request diff and in Brackets, so it's making it difficult for me to keep them in sync. Thanks.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Apr 08, 2013 at 19:05 GMT

---

sorry i'll certainly do so.
As to your question, encoding the semi-colon also would break things because of the order of replacements.
With something like that:

function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#").replace(";", "&#59;");
}

we would break the whole functionality because the replace would replace the semi-colons from the previous entities.
If we would reverse the order like:

function _encodeValue(value) {
        return value.replace("&", "&").replace(";", "&#59;").replace("#", "#");
}

the hash of the semi-colon entity would be replaced.

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 19:41 GMT

---

Regarding "encoding the semi-colon": Unfortunately, it's not easy to do, but, if it's required, you just have to be smarter about it. In this code, I don't think it's required because _encodeValue() is only used to encode query strings. If so, then give it a slightly different name such as _encodeQueryValue() and add a comment explaining why it's not an exact match for _decodeValue().

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 19:41 GMT

---

Done with initial review.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Apr 08, 2013 at 19:52 GMT

---

actually _encodeValue() is used elsewhere to encode the ui string, but encoding the semi-colon is actually not needed as far as i can see. I have no idea why i did need it once ^.
So i'll just get rid of it.

[core-ai-bot]

Comment by WebsiteDeveloper
Monday Apr 08, 2013 at 20:07 GMT

---

@redmunds fixes pushed.

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 21:49 GMT

---

In the sorting function, I noticed this after I merged the original code: this line of code:

            return (num1 === num2) ? 0 : (num1 > num2) ? 1 : -1;

Can be simplified to:

            return (num1 - num2);

This is because sort functions aren't limited to returning only -1, 0, or 1. They just need to return 0 if equal, or a positive or negative number to indicate greater than or less than.

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 22:11 GMT

---

Done with second review.

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Apr 09, 2013 at 04:36 GMT

---

@redmunds changes pushed.

[core-ai-bot]

Comment by redmunds
Tuesday Apr 09, 2013 at 04:58 GMT

---

Looks good. Merging.