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

Adding a test to validate line numbers and fix for multiline strings #217

Merged
merged 13 commits into from
May 7, 2023

Conversation

orchestr7
Copy link
Owner

@orchestr7 orchestr7 commented May 5, 2023

What's done:

  • On multiline strings we forgot to update isMultiline flag while decoding and parsing
  • Adding extra info to prettyPrint
  • Adding a test for validation of line numbers
  • Removing deprecated 'content' from the TomlNode
  • Added newline alignment for multiline strings (newlines after/before quotes-separators)

else ->
throw InternalDecodingException(
"Node of type [${node::class}] should not be processed in TomlDecoder.decodeValue(): <${node.content}>."
"Node of type [${node::class}] should not be processed in TomlDecoder.decodeValue(): <${node}>."

Check failure

Code scanning / ktlint

[STRING_TEMPLATE_CURLY_BRACES] string template has redundant curly braces: ${node}

[STRING_TEMPLATE_CURLY_BRACES] string template has redundant curly braces: ${node}
@@ -328,6 +292,10 @@
.writeNode(this)
.replace(" = ", "=")

internal fun print(emitLine: Boolean = false): String =
"${this::class.simpleName} ($this)${if (emitLine) "[line:${this.lineNo}]" else ""}\n"

Check failure

Code scanning / ktlint

[TOO_MANY_BLANK_LINES] too many consecutive blank lines: do not use more than two consecutive blank lines

[TOO_MANY_BLANK_LINES] too many consecutive blank lines: do not use more than two consecutive blank lines
node.children.forEach { child ->
prettyPrint(child, result, level + 1)
prettyPrint(child, result, emitLine, level + 1, )

Check failure

Code scanning / ktlint

[WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: ) should have 0 space(s) before, but has 1 space(s) before

[WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: ) should have 0 space(s) before, but has 1 space(s) before
else ->
throw InternalDecodingException(
"Node of type [${node::class}] should not be processed in TomlDecoder.decodeValue(): <${node.content}>."
"Node of type [${node::class}] should not be processed in TomlDecoder.decodeValue(): <${node}>."

Check failure

Code scanning / ktlint

[STRING_TEMPLATE_CURLY_BRACES] string template has redundant curly braces: ${node}

[STRING_TEMPLATE_CURLY_BRACES] string template has redundant curly braces: ${node}
@@ -328,6 +292,10 @@
.writeNode(this)
.replace(" = ", "=")

internal fun print(emitLine: Boolean = false): String =
"${this::class.simpleName} ($this)${if (emitLine) "[line:${this.lineNo}]" else ""}\n"

Check failure

Code scanning / ktlint

[TOO_MANY_BLANK_LINES] too many consecutive blank lines: do not use more than two consecutive blank lines

[TOO_MANY_BLANK_LINES] too many consecutive blank lines: do not use more than two consecutive blank lines
node.children.forEach { child ->
prettyPrint(child, result, level + 1)
prettyPrint(child, result, emitLine, level + 1, )

Check failure

Code scanning / ktlint

[WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: ) should have 0 space(s) before, but has 1 space(s) before

[WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: ) should have 0 space(s) before, but has 1 space(s) before
### What's done:
- On multiline strings we forgot to update isMultiline flag while decoding and parsing
- Adding extra info to prettyPrint
- Adding a test for validation of line numbers
- Removing deprecated 'content' from the TomlNode
@orchestr7 orchestr7 force-pushed the bugfix/lineno_literal branch from 5ef7f11 to c92b3e0 Compare May 5, 2023 01:47
| - TomlArrayOfTables ([[a.b]])[line:10]
| - TomlArrayOfTablesElement (technical_node)[line:10]
| - TomlKeyValuePrimitive (test=1)[line:11]
| - TomlKeyValuePrimitive (mls=''' 1
Copy link
Owner Author

Choose a reason for hiding this comment

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

that's a little bit weird, cause it looks like we forget to put a newline after multiline string delimiters

Copy link
Owner Author

@orchestr7 orchestr7 May 7, 2023

Choose a reason for hiding this comment

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

@NightEule5 while we encode the string, I think it is necessary for the user to have a proper formatting of multiline strings. Now we have the following:

a = """1
2
3"""

I think (even the TOML spec is confusing here) that we should add a newline there (it will be more convenient):

a = """
1
2
3
"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, true

Copy link
Owner Author

Choose a reason for hiding this comment

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

And of course I have forgotten to trim spaces before the quotes ''' while decoding 😢
#221

### What's done:
- Added newline alignment for multiline strings
@orchestr7 orchestr7 force-pushed the bugfix/lineno_literal branch from 41ab7a0 to ef6e3d9 Compare May 7, 2023 18:27
@orchestr7 orchestr7 requested a review from NightEule5 May 7, 2023 18:30
@@ -180,7 +180,9 @@ public abstract class TomlEmitter(config: TomlOutputConfig) {
val quotes = if (isLiteral) "'''" else "\"\"\""

emit(quotes)
.emitNewLine()
Copy link
Owner Author

@orchestr7 orchestr7 May 7, 2023

Choose a reason for hiding this comment

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

@NightEule5 can you please review it, and remind me please if I have forgotten something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks ok

@orchestr7 orchestr7 merged commit 76bcae0 into main May 7, 2023
@orchestr7 orchestr7 deleted the bugfix/lineno_literal branch May 7, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants