Skip to content
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

Add validation to new settings editor #56176

Merged
merged 16 commits into from
Aug 14, 2018

Conversation

JacksonKearl
Copy link
Contributor

@JacksonKearl JacksonKearl commented Aug 10, 2018

Closes #55801

@JacksonKearl JacksonKearl changed the title Settings validation Add validation to new settings editor Aug 10, 2018
@JacksonKearl JacksonKearl self-assigned this Aug 10, 2018
@JacksonKearl
Copy link
Contributor Author

Looks like there's some tests that need updating

@JacksonKearl
Copy link
Contributor Author

Never mind, looks like that's unrelated, despite being a tree issue.

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

There are some merge conflicts, some things in settingsTree.ts moved to settingsTreeModels.ts, I can help you fix it up if needed.

} else if (isArray(setting.type) && setting.type.indexOf('null') > -1 && setting.type.length === 2) {
if (setting.type.indexOf('number') > -1) {
element.valueType = 'nullable-integer';
} else if (setting.type.indexOf('number') > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

number vs integer?

Copy link
Member

Choose a reason for hiding this comment

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

And what about types that fall out of this check and end up with no valueType?

@@ -909,8 +909,7 @@ class SettingsContentBuilder {

setting.descriptionRanges = [];
const descriptionPreValue = indent + '// ';
for (let line of setting.description) {
// Remove setting link tag
for (let line of (setting.deprecationMessage ? [setting.deprecationMessage] : setting.description)) {
Copy link
Member

Choose a reason for hiding this comment

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

It only writes the deprecation message instead of the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could make it write both, but the concern from earlier was that it was not totally clear that the item was deprecated. Maybe it would be better with the deprecation message at the top? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should have both. How about just prepending the deprecationMessage in * so it gets rendered in italics? And we can decide whether we want something fancier later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, this is the raw text model so we dont have italic support. In the new editor we use the special deprecation warning element anyways

@roblourens
Copy link
Member

createValidator probably deserves unit tests. (But don't let that block merging this)

@JacksonKearl
Copy link
Contributor Author

image

@roblourens I think this should be ready to go.

@roblourens roblourens merged commit 44fdd51 into microsoft:master Aug 14, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting validation in new settings editor
3 participants