[CLOSED] Validate editor preferences

[core-ai-bot]

Issue by redmunds
Thursday Mar 13, 2014 at 02:53 GMT
Originally opened as adobe/brackets#7186

---

This is for #7020.

I did a slight refactoring so editor prefs can be validated when ever they change. Currently only validate "tabSize" and "spaceUnits" which are numbers. All other prefs are boolean, so the only thing validation would do is to enforce default if user mucks up the prefs file.

I started a ValidationUtils module that might be useful in other places.

---

redmunds included the following code: https://github.com/adobe/brackets/pull/7186/commits

[core-ai-bot]

Comment by dangoor
Thursday Mar 13, 2014 at 13:08 GMT

---

I think the validation should be done in the PreferencesManager. Something like this:

PreferencesManager.definePreference(SPACE_UNITS, "number", DEFAULT_SPACE_UNITS, {
    validator: function (value) {
        return ValidationUtils.isIntegerInRange(value, 0, null)
    }
});

Then PreferencesSystem.get can validate on the way out and return the default value if the current value is invalid. This eliminates a lot of the change to Editor.js and also makes it so that JSLint.main doesn't need to duplicate the validation logic.

What do you think of that approach?

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 16:14 GMT

---

I like the validator approach -- nice suggestion! I'll implement that.

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 19:18 GMT

---

Changes pushed. Ready for another review.

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 21:10 GMT

---

@TomMalbran Thanks for the review. Changes pushed.

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 22:51 GMT

---

@TomMalbran Pushed more changes.

[core-ai-bot]

Comment by TomMalbran
Thursday Mar 13, 2014 at 22:59 GMT

---

The code looks good to me now.

[core-ai-bot]

Comment by redmunds
Thursday Mar 13, 2014 at 23:23 GMT

---

I'm seeing a PreferencesManager unit test fail, and should add some unit tests for the new validator functionality.

should find preferences in the project

Error: Expected 4 to be 92.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/PreferencesManager-test.js:137:62)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12

[core-ai-bot]

Comment by RaymondLim
Thursday Mar 13, 2014 at 23:42 GMT

---

@redmunds You may need to adjust the value in test file .brackets.json which is using 92 as spaceUnits and your validator only allows up to 10. As you adjust the value is test file, then you also need to adjust all the test cases using that specific value.

[core-ai-bot]

Comment by redmunds
Friday Mar 14, 2014 at 15:19 GMT

---

@RaymondLim Ah, thanks.

[core-ai-bot]

Comment by redmunds
Friday Mar 14, 2014 at 16:30 GMT

---

@dangoor I fixed broken unit test and added a unit test for validator. Ready for another review.

[core-ai-bot]

Comment by zaggino
Friday Mar 14, 2014 at 22:44 GMT

---

I've seen validators return only true/false. Wouldn't it be better if they returned true for success and a string for invalid? This string would be displayable directly to the user in the UI. With true/false implementation I can't really see a way how to tell user where's the problem - the value may be too big or it may be too small, or it may have invalid characters inside...

[core-ai-bot]

Comment by zaggino
Friday Mar 14, 2014 at 22:46 GMT

---

Or in some libraries I've seen that validators return object such as this:

{
    valid: true/false,
    message: present when valid is false
}

[core-ai-bot]

Comment by redmunds
Friday Mar 14, 2014 at 23:04 GMT

---

@zaggino Since we run validator on both get() and set(), I think we'll also need to return value, so maybe:

{
    valid: true/false,
    value: value,
    message: present when valid is false
}

Calls to set() can ignore this, but this will affect every call to get(), so it's a good-size change.

@dangoor What do you think?

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 23:15 GMT

---

Instead of returning a message with the error, which would change depending on the error, maybe we can use and information/help string, something line The value must be an integer between 0 and 10 which doesn't change depending on the error. We can even use this string in the preferences dialog as the title or similar.

[core-ai-bot]

Comment by dangoor
Monday Mar 17, 2014 at 13:04 GMT

---

Perhaps we could set the message on definePreference and then have a separate validate call on PreferencesSystem that is used by the UI to validate the value whenever it wants (potentially even on each keystroke)