Add TOON v2 format support #3
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces TOON v2 format support alongside v1 backward compatibility. Changes include: new inline array header syntax ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Co-authored-by: tnfssc <29162020+tnfssc@users.noreply.github.com>
Co-authored-by: tnfssc <29162020+tnfssc@users.noreply.github.com>
Co-authored-by: tnfssc <29162020+tnfssc@users.noreply.github.com>
|
@copilot write and test comphrensively for both v1 and v2 formats of TOON. Default the output to v2 but add a flag that can output v1 as well |
Co-authored-by: tnfssc <29162020+tnfssc@users.noreply.github.com>
Added comprehensive tests for both V1 and V2 formats and a Usage: // V2 (default) - inline primitive arrays
data, _ := toon.Marshal(obj, toon.EncodeOptions{IndentSize: 2})
// Output: tags[3]: a,b,c
// V1 - list-style arrays
data, _ := toon.Marshal(obj, toon.EncodeOptions{IndentSize: 2, Version: toon.V1})
// Output: tags: [3|]\n - a\n - b\n - cNew tests in Commit: 2184b1c |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/toon/encoder.go (1)
337-431: Code duplication acknowledged but acceptable for now.There's substantial structural similarity between the V1 and V2 encoding functions (
encodeArrayWithKeyvsencodeArrayWithKeyV1,encodeArrayvsencodeArrayV1). The PR description notes this was reviewed and marked acceptable. Consider extracting shared logic into helper functions in a future refactor to improve maintainability.pkg/toon/marshal_test.go (1)
350-377: Consider adding edge case for trailing backslash.The escape sequence tests are comprehensive, but consider adding a test case for a trailing backslash at the end of the string (e.g.,
"hello\\"→"hello\"), which could expose edge cases in the character-by-character parsing loop.{"trailing backslash", `"hello\\"`, "hello\\"},pkg/toon/version_test.go (1)
288-306: Consider extracting repeated array conversion logic into a helper.The same type-switch pattern for handling
[]interface{}vsJsonArrayis repeated ~15 times throughout the file. A helper function would reduce duplication and improve maintainability.// toSlice converts JsonArray or []interface{} to []interface{} func toSlice(t *testing.T, v interface{}) []interface{} { t.Helper() switch arr := v.(type) { case []interface{}: return arr case JsonArray: result := make([]interface{}, len(arr)) for i, item := range arr { result[i] = item } return result default: t.Fatalf("Expected array type, got %T", v) return nil } }pkg/toon/parser.go (1)
192-197: Dead code:decodeArrayFromHeaderinline handling is now unreachable.The inline array path is now handled directly in
decodeValueFromLines(lines 40-48) anddecodeKeyValue(lines 133-141) beforedecodeArrayFromHeaderis called. The TODO and error on lines 194-196 are never reached.Consider removing the dead inline handling or updating the function signature:
func decodeArrayFromHeader(header *ArrayHeaderInfo, inlineValues string, cursor *LineCursor, baseDepth int, options DecodeOptions) (JsonArray, error) { - if inlineValues != "" { - // Inline primitive array - // TODO: Implement inline array decoding - return nil, fmt.Errorf("inline arrays not yet implemented") - } - if len(header.Fields) > 0 { return decodeTabularArray(header, cursor, baseDepth, options) } return decodeListArray(header, cursor, baseDepth, options) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/toon/encoder.go(6 hunks)pkg/toon/marshal_test.go(3 hunks)pkg/toon/parser.go(4 hunks)pkg/toon/parser_utils.go(1 hunks)pkg/toon/types.go(2 hunks)pkg/toon/version_test.go(1 hunks)tests/test.toon(1 hunks)tests/test_advanced.toon(1 hunks)tests/test_parity.toon(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/toon/encoder.go (1)
pkg/toon/types.go (3)
EncodeOptions(42-45)V1(32-32)IndentSize(12-12)
pkg/toon/marshal_test.go (2)
pkg/toon/marshal.go (1)
Unmarshal(31-38)pkg/toon/types.go (2)
DecodeOptions(36-39)IndentSize(12-12)
pkg/toon/parser.go (2)
pkg/toon/parser_utils.go (6)
ParseArrayHeaderLineTOONv2(79-147)ParseDelimitedValues(264-310)ParsePrimitiveToken(18-74)ParseArrayHeaderLine(152-261)ParseInlineArray(313-334)IsArrayHeaderAfterHyphen(337-340)pkg/toon/types.go (1)
JsonArray(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
🔇 Additional comments (20)
tests/test_parity.toon (1)
3-7: LGTM!The tabular array header format update from
[3|name|role]to[3|]{name|role}:correctly follows the TOON v2 specification changes described in the PR objectives. The data rows remain properly formatted with pipe-delimited values.tests/test.toon (1)
1-11: LGTM!The test file correctly demonstrates TOON v2 format features:
contributors[2]:uses the v2 array header syntaxfeatures[3]: parser,encoder,clishows inline primitive array encoding- Object and scalar values follow proper v2 formatting
pkg/toon/encoder.go (5)
44-87: LGTM!The
needsQuotingfunction comprehensively covers edge cases for string quoting: empty strings, whitespace, reserved literals, numeric-like values, special characters, and control characters. The ordering of checks is logical.
89-98: LGTM!The escape sequence order is correct—backslash is escaped first (line 92) before other characters, which prevents double-escaping issues.
170-193: LGTM!The value type branching is well-structured:
- Objects: colon followed by newline and nested content
- Arrays: delegated to
encodeArrayWithKeyfor version-aware handling- Primitives: colon-space followed by value
100-119: LGTM!The
encodePrimitiveValuefunction correctly handles all expected primitive types with appropriate formatting. The default case provides a reasonable fallback, and callers are expected to filter non-primitives viaisPrimitivechecks.
433-438: LGTM!The version branching correctly defaults to V2 format (since
V2 = iotameans zero-value defaults to V2), with V1 requiring explicit opt-in. This aligns with the PR requirement to "default output to v2."tests/test_advanced.toon (1)
1-7: LGTM!The test file effectively demonstrates TOON v2 format features:
- Line 1: Inline primitive array with unquoted string values
- Line 2: String values that look like numbers are properly quoted (
"1","3.5")- Line 3: Tabular array header with column definitions
- Line 7: Nested inline array within an object
This provides good coverage for v2 encoding behavior.
pkg/toon/types.go (2)
25-33: LGTM!The
TOONVersiontype design is well thought out:
V2 = iotaensures zero-value defaults to V2 (the new default format)- Clear documentation distinguishes V2 (inline arrays) from V1 (list-style arrays)
- Dedicated type provides type safety for version handling
41-45: LGTM!The
Versionfield addition toEncodeOptionsis clean and non-breaking. Existing callers that don't specifyVersionwill automatically get V2 format due to the zero-value default.pkg/toon/marshal_test.go (3)
124-128: LGTM - TOON v2 quoting behavior validated.The test correctly expects numeric-like string
"123"to be quoted in the output, aligning with TOON v2 spec requirements.
230-234: LGTM - Updated to TOON v2 array header format.The test now expects
[5]:instead of the legacy[5|]format, correctly reflecting V2 default behavior.
237-270: Good coverage for both V2 inline and V1 legacy array formats.The tests validate:
- V2 inline format (
[3]: 10,20,30) inTestUnmarshalArray- Legacy list format (
[3|]with- item) inTestUnmarshalArrayLegacyListThis ensures backward compatibility while testing the new format.
pkg/toon/parser_utils.go (1)
76-147: LGTM - Well-structured TOON v2 header parser.The function correctly:
- Extracts length from
[N]bracket content- Determines delimiter (comma default, pipe, or tab)
- Parses optional
{field1,field2}segment- Enforces trailing colon requirement
The implementation aligns with the V2 specification described in the PR objectives.
pkg/toon/version_test.go (3)
702-719: LGTM - Version constant validation is sound.The tests correctly verify:
V2 == 0(default zero value for Go's iota)V1 == 1- Default
EncodeOptions.VersionequalsV2This ensures the zero-value behavior defaults to V2 as intended by the PR.
725-739: Good edge case coverage for special characters.The test validates that strings containing commas, colons, and quotes are properly quoted in V2 output, ensuring delimiter/syntax conflicts are handled.
807-821: I'll help you verify whether there's a corresponding decode test for null arrays. Let me search the codebase for relevant test coverage.
<function_calls>
#!/bin/bashCheck for decode tests related to null arrays
rg -n "null,null" --type=go -C 3
</function_calls>Let me also search for Unmarshal or decode tests that might handle this case:
<function_calls>
#!/bin/bashSearch for Unmarshal tests in the test file
rg -n "Unmarshal.*null" --type=go -C 2
</function_calls>Let me get a broader view of the test file structure:
<function_calls>
#!/bin/bashFind the test file and check its structure
fd "version_test.go" --type f
</function_calls>Let me search for tests that specifically decode or unmarshal null array formats:
<function_calls>
#!/bin/bashSearch for test functions that handle null decoding/unmarshaling
rg -n "func Test.[Uu]nmarshal.[Nn]ull|func Test.[Dd]ecode.[Nn]ull" --type=go
</function_calls>Let me search more broadly for Unmarshal tests:
<function_calls>
#!/bin/bashGet all test functions in version_test.go that involve Unmarshal
rg -n "func Test.Unmarshal|Unmarshal.[.*null" --type=go -A 5
</function_calls>pkg/toon/parser.go (3)
30-70: LGTM - Well-structured fallback chain for root array parsing.The implementation correctly attempts parsing in order:
- TOON v2 format (
[N]:with inline values or list/tabular)- Legacy format (
[N|])- Inline array (
[item1, item2])This ensures backward compatibility while prioritizing the new format.
111-152: LGTM - Clean integration of V2 array header detection in key-value parsing.The logic correctly:
- Detects
key[N]:orkey[N]{fields}:patterns- Extracts the key before the bracket
- Delegates to
ParseArrayHeaderLineTOONv2for header parsing- Handles inline values vs list/tabular formats appropriately
274-299: LGTM - Correct sibling depth calculation for list-item objects.The fix correctly sets
siblingDepth = baseDepth + 1to account for the indentation after-. The added check on line 285 properly terminates object parsing when encountering a new list item at the same base depth.
| if obj, ok := item.(map[string]interface{}); ok { | ||
| // Special handling for object in list | ||
| if len(obj) == 0 { | ||
| // Empty object - just the hyphen (remove the space we added) | ||
| // Actually, per spec, empty object is just "-" on its own line | ||
| // We already wrote "- ", so we need to handle this better | ||
| // Let's trim the space we added | ||
| } else { |
There was a problem hiding this comment.
Empty object handling leaves trailing whitespace.
The comment on lines 231-234 acknowledges that empty objects should be formatted as just - without a trailing space, but the code block is empty and doesn't actually trim the space written on line 226. This results in - (with trailing space) instead of - for empty objects.
Consider implementing the trim or adjusting the write logic:
- sb.WriteString("- ")
-
if obj, ok := item.(map[string]interface{}); ok {
- // Special handling for object in list
if len(obj) == 0 {
- // Empty object - just the hyphen (remove the space we added)
- // Actually, per spec, empty object is just "-" on its own line
- // We already wrote "- ", so we need to handle this better
- // Let's trim the space we added
+ sb.WriteString("-")
} else {
+ sb.WriteString("- ")
keys := make([]string, 0, len(obj))Note: This same pattern appears in encodeArrayWithKeyV1 (line 350), encodeArray (line 464), and encodeArrayV1 (line 572).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/toon/encoder.go around lines 228 to 235, the code comments note empty
objects should render as "-" but the code doesn't remove the trailing space
already written, leaving "- " in output; update the logic to avoid writing the
extra space for empty objects (either detect empty map before writing the " - "
prefix and write "\n-" alone, or write the "- " as now and explicitly remove the
last space byte when obj is empty), and apply the same fix to the equivalent
blocks in encodeArrayWithKeyV1 (around line 350), encodeArray (around line 464),
and encodeArrayV1 (around line 572) so empty objects produce a single "-" with
no trailing space.
| // Handle quoted strings with escape sequences | ||
| if strings.HasPrefix(token, "\"") && strings.HasSuffix(token, "\"") && len(token) >= 2 { | ||
| inner := token[1 : len(token)-1] | ||
| // Unescape: process escape sequences character by character | ||
| var result strings.Builder | ||
| i := 0 | ||
| for i < len(inner) { | ||
| if inner[i] == '\\' && i+1 < len(inner) { | ||
| switch inner[i+1] { | ||
| case '\\': | ||
| result.WriteByte('\\') | ||
| i += 2 | ||
| case '"': | ||
| result.WriteByte('"') | ||
| i += 2 | ||
| case 'n': | ||
| result.WriteByte('\n') | ||
| i += 2 | ||
| case 'r': | ||
| result.WriteByte('\r') | ||
| i += 2 | ||
| case 't': | ||
| result.WriteByte('\t') | ||
| i += 2 | ||
| default: | ||
| // Unknown escape, keep as is | ||
| result.WriteByte(inner[i]) | ||
| i++ | ||
| } | ||
| } else { | ||
| result.WriteByte(inner[i]) | ||
| i++ | ||
| } | ||
| } | ||
| return result.String() | ||
| } |
There was a problem hiding this comment.
Byte-level iteration may corrupt multi-byte UTF-8 characters.
The escape sequence parser iterates over inner using byte indices (inner[i]), but non-ASCII characters in the string occupy multiple bytes. If a backslash precedes a multi-byte character or appears mid-sequence, the logic may misinterpret bytes.
Consider using []rune for character-by-character iteration:
- inner := token[1 : len(token)-1]
- // Unescape: process escape sequences character by character
- var result strings.Builder
- i := 0
- for i < len(inner) {
- if inner[i] == '\\' && i+1 < len(inner) {
- switch inner[i+1] {
+ inner := []rune(token[1 : len(token)-1])
+ var result strings.Builder
+ i := 0
+ for i < len(inner) {
+ if inner[i] == '\\' && i+1 < len(inner) {
+ switch inner[i+1] {
case '\\':
- result.WriteByte('\\')
+ result.WriteRune('\\')
i += 2
case '"':
- result.WriteByte('"')
+ result.WriteRune('"')
i += 2
// ... similar changes for other cases
default:
- result.WriteByte(inner[i])
+ result.WriteRune(inner[i])
i++
}
} else {
- result.WriteByte(inner[i])
+ result.WriteRune(inner[i])
i++
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle quoted strings with escape sequences | |
| if strings.HasPrefix(token, "\"") && strings.HasSuffix(token, "\"") && len(token) >= 2 { | |
| inner := token[1 : len(token)-1] | |
| // Unescape: process escape sequences character by character | |
| var result strings.Builder | |
| i := 0 | |
| for i < len(inner) { | |
| if inner[i] == '\\' && i+1 < len(inner) { | |
| switch inner[i+1] { | |
| case '\\': | |
| result.WriteByte('\\') | |
| i += 2 | |
| case '"': | |
| result.WriteByte('"') | |
| i += 2 | |
| case 'n': | |
| result.WriteByte('\n') | |
| i += 2 | |
| case 'r': | |
| result.WriteByte('\r') | |
| i += 2 | |
| case 't': | |
| result.WriteByte('\t') | |
| i += 2 | |
| default: | |
| // Unknown escape, keep as is | |
| result.WriteByte(inner[i]) | |
| i++ | |
| } | |
| } else { | |
| result.WriteByte(inner[i]) | |
| i++ | |
| } | |
| } | |
| return result.String() | |
| } | |
| // Handle quoted strings with escape sequences | |
| if strings.HasPrefix(token, "\"") && strings.HasSuffix(token, "\"") && len(token) >= 2 { | |
| inner := []rune(token[1 : len(token)-1]) | |
| var result strings.Builder | |
| i := 0 | |
| for i < len(inner) { | |
| if inner[i] == '\\' && i+1 < len(inner) { | |
| switch inner[i+1] { | |
| case '\\': | |
| result.WriteRune('\\') | |
| i += 2 | |
| case '"': | |
| result.WriteRune('"') | |
| i += 2 | |
| case 'n': | |
| result.WriteRune('\n') | |
| i += 2 | |
| case 'r': | |
| result.WriteRune('\r') | |
| i += 2 | |
| case 't': | |
| result.WriteRune('\t') | |
| i += 2 | |
| default: | |
| // Unknown escape, keep as is | |
| result.WriteRune(inner[i]) | |
| i++ | |
| } | |
| } else { | |
| result.WriteRune(inner[i]) | |
| i++ | |
| } | |
| } | |
| return result.String() | |
| } |
🤖 Prompt for AI Agents
In pkg/toon/parser_utils.go around lines 36 to 71, the escape-sequence
unescaping iterates bytes which can corrupt multi-byte UTF-8 runes; convert
inner to a []rune and iterate rune-by-rune so you treat Unicode characters as
single code points, perform escape lookahead on the next rune (not next byte),
write runes to the strings.Builder (use WriteRune) and when encountering unknown
escapes emit the backslash and the following rune unchanged; ensure you advance
the rune index correctly for two-rune escapes and handle end-of-input safely.
There was a problem hiding this comment.
Pull request overview
This PR implements support for TOON v2 format specification, introducing inline primitive array syntax while maintaining backward compatibility with TOON v1. The default behavior now uses V2 format (key[3]: a,b,c), but V1 format (key: [3|] with list items) is still available via the Version option.
Key changes:
- V2 format uses inline arrays for primitives (
[N]: values) and list format only for complex types - Added string quoting logic to prevent ambiguity with numbers/booleans
- Implemented character-by-character escape sequence processing for proper
\\nvs\nhandling
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/toon/types.go | Added TOONVersion type with V1/V2 constants and Version field to EncodeOptions |
| pkg/toon/encoder.go | Implemented V2 inline array encoding, string quoting logic, and separate V1 encoding functions |
| pkg/toon/parser.go | Added V2 format parsing with inline array support and improved object-in-list depth handling |
| pkg/toon/parser_utils.go | Added ParseArrayHeaderLineTOONv2 function and character-by-character escape sequence handling |
| pkg/toon/marshal_test.go | Updated tests for V2 format expectations and added escape sequence tests |
| pkg/toon/version_test.go | Comprehensive test suite covering V1/V2 encoding, decoding, and round-trip scenarios |
| tests/*.toon | Updated test files to use V2 format syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // isPrimitive checks if a value is a primitive (not object or array) | ||
| func isPrimitive(v interface{}) bool { | ||
| switch v.(type) { | ||
| case map[string]interface{}, []interface{}: | ||
| return false | ||
| default: | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The isPrimitive function doesn't handle JsonObject and JsonArray types, which are defined in types.go. Since these are type aliases for map[string]JsonValue and []JsonValue, they won't match the case statement and will incorrectly be classified as primitives. Add cases for JsonObject and JsonArray types.
pkg/toon/encoder.go
Outdated
| sb.WriteString("- ") | ||
|
|
||
| if obj, ok := item.(map[string]interface{}); ok { | ||
| // Special handling for object in list | ||
| if len(obj) == 0 { | ||
| // Empty object - just the hyphen (remove the space we added) | ||
| // Actually, per spec, empty object is just "-" on its own line | ||
| // We already wrote "- ", so we need to handle this better | ||
| // Let's trim the space we added |
There was a problem hiding this comment.
The comment indicates incomplete handling of empty objects. The code writes \"- \" on line 226 but doesn't remove the trailing space for empty objects as the comment suggests. Either implement the intended behavior or remove the misleading comment.
| sb.WriteString("- ") | |
| if obj, ok := item.(map[string]interface{}); ok { | |
| // Special handling for object in list | |
| if len(obj) == 0 { | |
| // Empty object - just the hyphen (remove the space we added) | |
| // Actually, per spec, empty object is just "-" on its own line | |
| // We already wrote "- ", so we need to handle this better | |
| // Let's trim the space we added | |
| if obj, ok := item.(map[string]interface{}); ok { | |
| // Special handling for object in list | |
| if len(obj) == 0 { | |
| // Empty object - just the hyphen, per spec | |
| sb.WriteString("-") |
| // encodeArrayWithKey encodes an array as a value for a key (key already written, need to write header and content) | ||
| func encodeArrayWithKey(sb *strings.Builder, arr []interface{}, depth int, options EncodeOptions) error { |
There was a problem hiding this comment.
There is significant code duplication between encodeArrayWithKey (lines 195-335), encodeArray (lines 433-557), and their V1 counterparts. The object-in-list encoding logic (lines 228-297, 463-524, etc.) is nearly identical across multiple functions. Consider extracting this into a shared helper function to improve maintainability and reduce the risk of inconsistencies.
pkg/toon/marshal_test.go
Outdated
| {"simple tab", `"hello\tworld"`, "hello\tworld"}, | ||
| {"simple quote", `"hello\"world"`, "hello\"world"}, | ||
| {"simple backslash", `"hello\\world"`, "hello\\world"}, | ||
| {"backslash then n", `"hello\\nworld"`, "hello\\nworld"}, |
There was a problem hiding this comment.
[nitpick] Test case name 'backslash then n' is ambiguous. This tests the sequence \\n which should be interpreted as a literal backslash followed by 'n', not a newline. Consider renaming to 'escaped backslash then n' or 'literal backslash-n' to clarify the intent.
| {"backslash then n", `"hello\\nworld"`, "hello\\nworld"}, | |
| {"literal backslash-n", `"hello\\nworld"`, "hello\\nworld"}, |
pkg/toon/encoder.go
Outdated
| sb.WriteString("- ") | ||
|
|
||
| if obj, ok := item.(map[string]interface{}); ok { | ||
| if len(obj) == 0 { | ||
| // Empty object - remove the trailing space from "- " | ||
| } else { |
There was a problem hiding this comment.
Similar to Comment 2, the code writes \"- \" but the comment indicates the trailing space should be removed for empty objects. This same incomplete handling appears in multiple places (lines 230-235, 464-466, 572-574). Implement consistent empty object handling across all occurrences.
| sb.WriteString("- ") | |
| if obj, ok := item.(map[string]interface{}); ok { | |
| if len(obj) == 0 { | |
| // Empty object - remove the trailing space from "- " | |
| } else { | |
| if obj, ok := item.(map[string]interface{}); ok { | |
| if len(obj) == 0 { | |
| // Empty object - remove the trailing space from "- " | |
| sb.WriteString("-") | |
| } else { | |
| sb.WriteString("- ") |
|
@copilot Address the comments dropped in the PR |
… test name Co-authored-by: tnfssc <29162020+tnfssc@users.noreply.github.com>
Addressed all PR review comments:
Commit: df22f92 |
[N|]format to[N]for default comma delimiter[N]:format for comma delimiter (instead of[N|])key[N]: v1,v2,v3)ParseArrayHeaderLineTOONv2for new formatOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.