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

Added initialPosition and launchMode to settings schema #4222

Merged
merged 8 commits into from
Jan 28, 2020

Conversation

cinnamon-msft
Copy link
Contributor

Summary of the Pull Request

Added initialPosition and launchMode to settings json schema and md file

References

PR Checklist

  • Closes Settings Schema is Missing Initial Position and Launch Mode #3290
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@cinnamon-msft
Copy link
Contributor Author

I know the regex for the coordinates is wrong, currently working to figure it out.

@modscleo4
Copy link

Try using ^(\d+),[ ]?(\d+)$ for Launch position, I think it's better

@cinnamon-msft cinnamon-msft requested a review from a team January 28, 2020 18:55
@@ -10,7 +10,7 @@
"format": "color"
},
"Coordinates": {
"pattern": "^(-?\\d+)?,?(-?\\d+)?$",
"pattern": "^(?!-?\\d+-\\d+$)(-?\\d+)?,?(-?\\d+)?$",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hold up, don't do that. Lookarounds are expensive.

The correct way to fix this is -- assuming you want to support 5 5,5 and ,5 -- to include the , inside one of the optional groups.

Use this pattern.

^(-?\\d+)?(,?\\s?-?\\d+)?$

Including the , in the second optional group (also: \s matches a single space, and is valuable to put in places users might put spaces) means you don't need to have an expensive negative lookaround in the regex that says "plx make sure it doesn't contain 5-5"

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT Jan 28, 2020

Choose a reason for hiding this comment

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

Using ? for \s makes it so that they can only say 5, 5 or 5,5 -- they can't say 5,<space space space space space>5. (edit: github ate the spaces)

Unfortunately, they can say 5,\n5 😁

Copy link
Contributor Author

@cinnamon-msft cinnamon-msft Jan 28, 2020

Choose a reason for hiding this comment

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

We also have to allow for 5, 😟

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is part of the thing I said ;P

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I'd go so far as to replace the ,? with , -- because the entire second part is optional already..

^(-?\\d+)?(,\\s?-?\\d+)?$ -- to be valid, the second part must have a ,. you cannot say 5 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, the pattern you have doesn't allow for 5,

Also, having "" will just use the default settings, so that's probably fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, 5, is legal.

^(-?\\d+)?(,\\s?(-?\\d+)?)?$

Copy link
Contributor

Choose a reason for hiding this comment

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

"if there's a second digit group it must be preceded by ,\s+ but there doesn't need to be digits after the ,"

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay yeah that works :) I'll add it in! Thanks 😄

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 28, 2020
@cinnamon-msft
Copy link
Contributor Author

Ah nope, I'll remove them now. They're in other places throughout the schema so I'll take them all out.

@cinnamon-msft cinnamon-msft merged commit 814f4ca into master Jan 28, 2020
@cinnamon-msft cinnamon-msft deleted the cinnamon/settings-schema-initial-position branch January 28, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings Schema is Missing Initial Position and Launch Mode
4 participants