Skip to content

fix: merge byte string literal#834

Closed
togami2864 wants to merge 2 commits into
apache:mainfrom
togami2864:fix/merge-byte-literal
Closed

fix: merge byte string literal#834
togami2864 wants to merge 2 commits into
apache:mainfrom
togami2864:fix/merge-byte-literal

Conversation

@togami2864

Copy link
Copy Markdown
Contributor

As the same as #812 (comment), quote style isn't important.
This PR merges SingleQuotedByteStringLiteral and DoubleQuotedByteStringLiteral to ByteStringLiteral.

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 4400773307

  • 21 of 23 (91.3%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 86.126%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 0 1 0.0%
tests/sqlparser_bigquery.rs 17 18 94.44%
Files with Coverage Reduction New Missed Lines %
src/parser.rs 1 83.0%
Totals Coverage Status
Change from base Build 4374743456: -0.005%
Covered Lines: 13496
Relevant Lines: 15670

💛 - Coveralls

@ankrgyl

ankrgyl commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

@alamb do you have thoughts on changes like this? I generally agree it's probably fine to only have one type of byte string literal, and the two new types are new (2/20/23) but there's a small risk of regressing someone's existing code that distinguishes the two cases.

@alamb

alamb commented Mar 16, 2023

Copy link
Copy Markdown
Contributor

I don't see the harm in keeping these two variants separate -- if downstream crates want to treat them the same they can do so, and if they want to handle them separately they can with the current forumulation

Thus I would not be in favor of merging this unless there is some compelling usecase

@togami2864

Copy link
Copy Markdown
Contributor Author

That makes sense to me. I'll close this PR.

@togami2864 togami2864 closed this Mar 17, 2023
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.

4 participants