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 note to README about double quoted/plain YAML string escape characters #273

Merged
merged 7 commits into from
Mar 3, 2021

Conversation

derekjobst
Copy link
Member

Due to the way yaml is decoded, \ characters must be escaped when used in strings with double quotes, but should not be escaped in plain style strings without quotes.

@derekjobst derekjobst added the documentation The issue is due to bad documentation label Mar 2, 2021
@derekjobst derekjobst requested a review from a team March 2, 2021 18:41
@bluekeyes
Copy link
Member

While including the note here works, many other predicates also use regular expressions. Maybe it would be beneficial to instead add a section like Notes on YAML Syntax to the README as a sub-heading of the policy.yml Specification section?

That would give space to be more expansive and add other notes in the future. For example, I just checked and single quoted strings are also valid and don't need escaping.

@derekjobst
Copy link
Member Author

That's a great idea. I'll add the section now. I'd be worried users might miss it when looking up a single regex based rule in the examples — what do you think of adding a note to comment_patterns and changed_files paths that repeat the difference and reference this new section?

@bluekeyes
Copy link
Member

The inline comment format isn't great for adding lots of information about each property and I'd be worried about duplication, but I think a line similar to See "Notes on YAML Syntax" for tips to correctly escape regular expressions above each relevant property would be fine.

README.md Outdated
@@ -61,6 +61,23 @@ Consider the following example, which allows changes to certain paths without
review, but all other changes require review from the `palantir/devtools`.
Any member of the `palantir` organization can also disapprove changes.

#### Notes on YAML Syntax
Copy link
Member

Choose a reason for hiding this comment

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

I'd place this after the sample YAML block and before the Remote Policy Configuration section. Right now, the previous text says "Consider the following example" but then this section appears before the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, good catch

README.md Outdated Show resolved Hide resolved
derekjobst and others added 2 commits March 2, 2021 13:18
Co-authored-by: Billy Keyes <bkeyes@palantir.com>
@derekjobst derekjobst merged commit c206d51 into develop Mar 3, 2021
@derekjobst derekjobst deleted the djobst/regex-quotes-docs branch March 3, 2021 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation The issue is due to bad documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants