Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Convert numbers to strings, so StringMatch can match it. #11484

Merged
merged 1 commit into from
Jul 27, 2015

Conversation

sprintr
Copy link
Contributor

@sprintr sprintr commented Jul 26, 2015

Converts "number" values into strings so StringMatch.stringMatch can match it.

@abose This is critical to go with the release.

@abose
Copy link
Contributor

abose commented Jul 26, 2015

@sprintr what are the repercussions if we do not have this? what will break/not break. Could you give more details about the problems this pr addresses.
@nethip

@sprintr
Copy link
Contributor Author

sprintr commented Jul 26, 2015

"number" type preferences when stringMatch(ed) will through an undefined is not a function and also number type preferences were not matched because of this since stringMatch expects string values to match against a query.

Came across this while writing docs for #11200. Have sent them to you via slack.

@abose
Copy link
Contributor

abose commented Jul 26, 2015

OK, that is serious in that it would break brackets when trying to edit preferences file?

@sprintr
Copy link
Contributor Author

sprintr commented Jul 26, 2015

Run this in brackets console to see what error get produced.

var StringMatch = require("utils/StringMatch");
StringMatch.stringMatch(1024, "10", {  preferPrefixMatches: true });

Converting 1024 to string works fine.

@sprintr
Copy link
Contributor Author

sprintr commented Jul 26, 2015

Yes it broke hints when I pressed enter. So this fixes that as well.

@abose
Copy link
Contributor

abose commented Jul 26, 2015

Thanks @sprintr .
Re- GM an RC build is a tough thing to push to the stakeholders as it is an expensive/labour intensive process. Several teams(brackets-dev, Release engineering, LOC testing, international translation vendors) & Lot of manual testing is involved prior to an RC build of brackets.- so just wanted to give reasons to justify that.
@nethip has to make the call.
If we are trigerring a rew release, I would also like to nominate #11485 - Health logs for find / replace in files, instant search usage.

@nethip
Copy link
Contributor

nethip commented Jul 27, 2015

@sprintr How often do the users run into this use case? I did not understand the workflow with the number preference. Could you tell me what are the exact steps to repro this (from the user point of view). As @abose mentioned, the release tasks are quite laborious. If the frequency of someone running this issue is very minimal, I would nominate this for the next release.

@sprintr
Copy link
Contributor Author

sprintr commented Jul 27, 2015

I don't know any ext using number preference but since brackets supported that even before this feature I believe we should support it here as well. Right now it is broken.

@nethip
Copy link
Contributor

nethip commented Jul 27, 2015

@sprintr OK! Could you mention the exact repro steps? Let me speak to the team and take an opinion.

@sprintr
Copy link
Contributor Author

sprintr commented Jul 27, 2015

You can copy any number type preference from the wiki page "'Preference code hints for developers". I am on phone so can't give exact steps to repro this

@@ -281,6 +281,13 @@ define(function (require, exports, module) {
return null;
}

// Convert integers to strings, so StringMatch.stringMatch can match it.
if (option.type === "number" || option.valueType === "number") {
values = values.map(function (val) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sprintr use $.map(values, function (val) { here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess raw ES5 should be faster

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this. I think the one you have above should work fine.

nethip added a commit that referenced this pull request Jul 27, 2015
Convert numbers to strings, so StringMatch can match it.
@nethip nethip merged commit 273804e into adobe:release Jul 27, 2015
@sprintr sprintr deleted the fix-pref-number-hints branch July 27, 2015 06:08
@sprintr
Copy link
Contributor Author

sprintr commented Jul 27, 2015

@nethip Thanks for getting this in. When is the release scheduled?

@nethip
Copy link
Contributor

nethip commented Jul 27, 2015

@sprintr No issues! Thanks for jumping in and fixing it. Really appreciate it. We are targeting today for doing the release.

@sprintr
Copy link
Contributor Author

sprintr commented Jul 27, 2015

It was nice to get this in time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants